Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add progress bar to phonon calculations #319

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

RastislavTuranyi
Copy link

Closes #268

Adds a progress bar to phonon calculations using tqdm, which is already a dependency. A new argument was added to Phonons.__init__ to control the presence of this feature, disabled by default to minimise impact on code in general. However, the progress bar is always enabled for CLI.

One issue that I and @ajjackson have noted is that the bar only shows the progress over displacements, meaning that when bands, DOS, or PDOS are chosen to be calculated, the 100% bar just continues sitting there despite the prolonged calculations. This could be confusing to users. One solution could be writing to stdout before/after the progress bar appears, saying what the bar is for, but we weren't sure what the best path forward is. What do you think?

Additionally, I should probably mention that I ran into #316 while working on this despite only running janus only in serial.

@@ -8,6 +8,7 @@
import phonopy
from phonopy.file_IO import write_force_constants_to_hdf5
from phonopy.structure.atoms import PhonopyAtoms
from tqdm import tqdm
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tqdm may be a dependency of a dependency and that's not a good thing to rely on. You need to add this explicitly to dependencies if you want to use it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, rich is also a dependency of a dependency, that may be a better choice here since Jupyter notebooks and IPython are encouraged ways of using Janus

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how rich compares in terms of performance/overheads?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, but I should think minimal in relation to a phonon calculation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supposedly, they should be pretty comparable nowadays. Regardless, as mentioned, the milliseconds are going to be negligible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all fine if everyone's happy. The main thing was that it was an option which is why (although not necessarily obvious in the GitHub font) "may" was italicised.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, it does appear to me that tqdm integration with jupyter notebook could be non-trivial

Don't worry the integration is that it prints to the output perfectly happily.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, to confirm, we are sticking with tqdm, is that right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I obviously haven't seen how this specific one would look in comparison, but in general rich maybe looks slightly nicer to me, and maybe fits stylistically with the typer interface better.

So, all things being equal (e.g. no immediately noticeable performance issues, which it sounds like there shouldn't be), I'd maybe tend towards rich.

I am completely fine with tqdm if it's more straightforward to finish this PR up though, as long as it addresses the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, don't forget @oerc0122's original point to add the dependency to pyproject.toml!

@ElliottKasoar
Copy link
Member

Thanks for this!

One issue that I and @ajjackson have noted is that the bar only shows the progress over displacements, meaning that when bands, DOS, or PDOS are chosen to be calculated, the 100% bar just continues sitting there despite the prolonged calculations. This could be confusing to users. One solution could be writing to stdout before/after the progress bar appears, saying what the bar is for, but we weren't sure what the best path forward is. What do you think?

I agree it's a bit confusing, so I'd say yes we probably do want some kind of output labelling the progress bar.

Additionally, I should probably mention that I ran into #316 while working on this despite only running janus only in serial.

Thanks for the report! If you still encounter this after updating with the latest fixes, please open a new issue.

@@ -258,6 +258,7 @@ def phonons(
"write_results": True,
"write_full": write_full,
"file_prefix": file_prefix,
"enable_progress_bar": True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"enable_progress_bar": True
"enable_progress_bar": True,

To make ruff happy (best merged with other suggestion)

@@ -149,6 +150,7 @@ def __init__(
write_results: bool = True,
write_full: bool = True,
file_prefix: Optional[PathLike] = None,
enable_progress_bar: bool = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enable_progress_bar: bool = False
enable_progress_bar: bool = False,

To make ruff happy (best merged with other suggestion)

@oerc0122
Copy link
Collaborator

oerc0122 commented Oct 4, 2024

I agree it's a bit confusing, so I'd say yes we probably do want some kind of output labelling the progress bar.

tqdm allows you to pass a label.

desc: str, optional
Prefix for the progressbar.

-- tqdm docs

@RastislavTuranyi
Copy link
Author

I am looking into rich at the moment, as per suggestion, and it can also have a label. The progress bar can also be made to disappear at the end, so I think a text could be displayed in its place to indicate that further calculations may be ongoing. Unfortunately, since the bands/DOS/etc. calculations are handled entirely by Phonopy, I don't think there is a way to display a progress bar there, though we could maybe do like a spinner or something of that sort, if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visual progress information during phonon calculations
3 participants