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.

Sign up for GitHub

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

Jump to bottom

Add progress bar to phonon calculations #319

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+9 −0
Open

Add progress bar to phonon calculations #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a89b5ee
Add tqdm progress bar to Phonons.calc_force_constants
Oct 2, 2024
0948421
Add documentation
Oct 2, 2024
3f85455
Merge branch 'main' into 268_phonon_progress_bar
Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions janus_core/calculations/phonons.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!


from janus_core.calculations.base import BaseCalculation
from janus_core.calculations.geom_opt import GeomOpt
Expand Down Expand Up @@ -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)

) -> None:
"""
Initialise Phonons class.
Expand Down Expand Up @@ -214,6 +216,8 @@ def __init__(
file_prefix : Optional[PathLike]
Prefix for output filenames. Default is inferred from structure name, or
chemical formula of the structure.
enable_progress_bar : bool
Whether to show a progress bar during phonon calculations. Default is False.
"""
(read_kwargs, minimize_kwargs) = none_to_dict((read_kwargs, minimize_kwargs))

Expand All @@ -229,6 +233,7 @@ def __init__(
self.plot_to_file = plot_to_file
self.write_results = write_results
self.write_full = write_full
self.enable_progress_bar = enable_progress_bar

# Ensure supercell is a valid list
self.supercell = [supercell] * 3 if isinstance(supercell, int) else supercell
Expand Down Expand Up @@ -357,6 +362,9 @@ def calc_force_constants(
phonon.generate_displacements(distance=self.displacement)
disp_supercells = phonon.supercells_with_displacements

if self.enable_progress_bar:
disp_supercells = tqdm(disp_supercells)

phonon.forces = [
self._calc_forces(supercell)
for supercell in disp_supercells
Expand Down
1 change: 1 addition & 0 deletions janus_core/cli/phonons.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

}

# Initialise phonons
Expand Down
Loading