-
Notifications
You must be signed in to change notification settings - Fork 48
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
Espresso Bands job and flow #1701
Espresso Bands job and flow #1701
Conversation
Can one of the admins verify this patch? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1701 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 79 80 +1
Lines 3055 3103 +48
=======================================
+ Hits 3037 3085 +48
Misses 18 18 ☔ View full report in Codecov by Sentry. |
Do not merge this yet. I have to tweak few things |
@Andrew-S-Rosen This is ready to be merged from my side, but I think we should discuss if there is an easy way to generate a good k-path from the system space group. This PR so far does not specify any path so the results are crap unless the user specify a proper k-path |
@Nekkrad: Great! I will review this shortly. I'll be traveling today and tomorrow so will get to it by the 15th. In the meantime, your question about k-path is a good one. There is a general-purpose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, @Nekkrad!!! Overall, this is in good shape. I just have a few logistical questions for you that might help with cleaning things up. My major comment is that I'm not sure we really need a separate bands_pp_job
--- it seems like we can pack that step at the end of bands_job
so users can just always have post-processed data, especially because the post-processing step is trivial in terms of compute cost.
I also think it would be good to provide a default k-path from the aforementioned Pymatgen module so this is usable out of the box.
@Andrew-S-Rosen Thanks! Regarding your questions:
I am willing to merge the bands_pp_job into bands_job, add k-path defaults with pymatgen. I can also remove the flow. |
@Nekkrad: Thanks for the info! Very helpful. If the fermi surface task is very cheap and always after a bandstructure calculation (which I assume it must be), then I would lean towards also having it be within the
Agreed.
Thanks! I think this would make the most sense. Of course, you are the most immediate end-user, so don't hesitate to let me know if the suggestion hinders your productivity in any way. Thank you for the contribution and discussion! |
QE has a lot of binaries... and I think you are right here to say that they probably do not all deserve a job. Initially I wanted each binary to have its own job because I believe it is very nice in a user experience point of view. But, considering other addons such as https://anharmonic.github.io/thermal2/ that contains more than 10 binaries alone... It might be well advised to do a "all_kind" job which would be able to run any kind of software using Espresso style input. Few constraints that might be annoying:
Also it might be fair enough to say "let the user create their own job to run these binaries". |
Thanks for the input! I'm not opposed to having a bunch of individual jobs if you feel like people will use them frequently enough. In this case, my comment was mostly about the post-processing step, which I felt like could safely be done as part of the parent DOS job. Re: the "all" style of job, it's an interesting idea. Certainly, I can see value for it. One challenge that comes to mind that wasn't mentioned is that different binaries might have different sensible defaults as parameters, in which case one might as well separate them into individual jobs. Also, requiring users to provide a parsing function is quite messy. Indeed, under those scenarios, we might as well just assume that quacc is providing the infrastructure for people to do things on their own if they so choose. @Nekkrad: This is all separate from the current PR, which I think looks great and just requires some minor adjustments so that users aren't running a dedicated post-processing step unnecessarily (e.g. potentially as a separate Slurm job) and a sensible default for the k-path. Let me know if you need any help on either. 👍 |
@Andrew-S-Rosen Sorry been pretty busy. Everything is good but I have a couple of questions
|
@Nekkrad: This is a fair point. Perhaps it would make sense to have the default route be to: 1) make a primitive cell; 2) generate the pymatgen k-path. We could mention in the docs that this is what happens. If the user does wants to explicitly provide a k-path, then no transformation would be done on the structure. I'd avoid trying to map the primitive path to the conventional path --- seems messy.
Apologies --- I'm trying to find my comment about the slab flow you mention but am having trouble seeing it. In any case, there are basically two options:
The first option only makes sense if there are scenarios where people would want to run a standalone fermi surface and/or post-processing job outside of the DOS calculation. If that's not the case, then option 2 makes more sense. But both are doing the same thing really. |
@Andrew-S-Rosen The issues are
|
https://gitlab.com/ase/ase/-/merge_requests/3263 will address the first point mentioned by @Nekkrad without the need of adding any keys (that Ask does not really want) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Nekkrad! This looks very nice. I have left some comments.
There is no "fs" in the espresso_key.py in ASE. I will open a PR later on today. This prevents us from running any fermi_surface calculations
I merged the MR in ASE mentioned by @tomdemeyere. Next time you make a commit to this PR, the updated ASE will be installed.
The function "_cleanup_params" here check whether we are using both kspacing and kpts. The problem occurs if .get("kpts") is a list cause it will raise Value error.
Apologies. The issue here isn't immediately clear to me yet. Feel free to make an edit or perhaps if you wouldn't mind explaining a bit more, I can help.
src/quacc/recipes/espresso/bands.py
Outdated
prev_dir: str | Path, | ||
run_bands_pp: bool = True, | ||
run_fermi_surface: bool = False, | ||
primitive: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this would be more accurate to say make_bandpath: bool = True
and then mention it will convert to primitive in the docstring.
src/quacc/recipes/espresso/bands.py
Outdated
If True, a fs.x calculation will be carried out. | ||
This allows to generate the fermi surface of your structure. | ||
primitive | ||
If True, it gets the primitive cell for your structure and generate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"generate" --> "generates"
src/quacc/recipes/espresso/bands.py
Outdated
This allows to generate the fermi surface of your structure. | ||
primitive | ||
If True, it gets the primitive cell for your structure and generate | ||
the high symmetry k-path using Latimer-Munro approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"using Latminer-Munro" --> "using the Latmer-Munro"
src/quacc/recipes/espresso/bands.py
Outdated
If True, it gets the primitive cell for your structure and generate | ||
the high symmetry k-path using Latimer-Munro approach. | ||
For more information look at | ||
https://pymatgen.org/pymatgen.symmetry.html#pymatgen.symmetry.bandstructure.HighSymmKpath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use [pymatgen.symmetry.bandstructure.HighSymmKpath][]
since it will auto hyperlink in the docs.
src/quacc/recipes/espresso/bands.py
Outdated
See the type-hint for the data structure. | ||
""" | ||
|
||
calc_defaults = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this and simply leave calc_defaults
as the default in base_fn
. Or just put {}
in the base_fn
call directly.
src/quacc/recipes/espresso/bands.py
Outdated
|
||
results["bands"] = bands_result | ||
results["bands_pp"] = bands_pp_results if run_bands_pp else None | ||
results["fermi_surface"] = fermi_results if run_fermi_surface else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this into the corresponding if
statement that way we don't have the entry if the job isn't run.
src/quacc/recipes/espresso/bands.py
Outdated
|
||
from quacc.schemas._aliases.ase import RunSchema | ||
|
||
class BandsSchema(TypedDict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do BandsSchema(TypedDict, total=False)
to indicate that not all three entries may necessarily be present.
src/quacc/recipes/espresso/dos.py
Outdated
@@ -36,6 +35,11 @@ class ProjwfcSchema(TypedDict): | |||
non_scf_job: RunSchema | |||
projwfc_job: RunSchema | |||
|
|||
class BandsSchema(TypedDict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed now.
src/quacc/recipes/espresso/bands.py
Outdated
) | ||
|
||
|
||
def _fermi_surface( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this and the other tasks, perhaps what would be best would be to make them all @job
s (and remove the _
in front). Then, when they are called within bands_job
, you can do
from quacc.wflow_tools.customizers import strip_decorator
strip_decorator(fermi_surface)
Then one could run the task as a standalone job if they wish. If you do this, feel free to add the jobs to the recipe list too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Fermi surface task isn't covered, so we'll want to address this.
Name changed, tell me if you like them. Not very inspired but probably ok ahah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- docs/user/recipes/recipes_list.md (1 hunks)
- src/quacc/calculators/espresso/espresso.py (4 hunks)
- src/quacc/recipes/espresso/_base.py (2 hunks)
- src/quacc/recipes/espresso/bands.py (1 hunks)
- src/quacc/recipes/espresso/dos.py (2 hunks)
- src/quacc/settings.py (1 hunks)
- tests/core/recipes/espresso_recipes/test_bands.py (1 hunks)
- tests/core/recipes/espresso_recipes/test_dos.py (1 hunks)
Files skipped from review due to trivial changes (1)
- tests/core/recipes/espresso_recipes/test_dos.py
Additional comments: 14
tests/core/recipes/espresso_recipes/test_bands.py (2)
- 18-43: The test
test_bands_job
correctly sets up and executes a bands job, asserting the expected outcomes. However, it's important to ensure that the test covers all relevant aspects of the bands job functionality, including error handling and edge cases.- 52-86: The test
test_bands_job_with_fermi
extends the testing to include Fermi surface calculations. It's well-structured and asserts the key outcomes. To further improve test coverage, consider adding tests for scenarios where the Fermi surface calculation might fail or produce unexpected results.src/quacc/recipes/espresso/_base.py (1)
- 202-204: The conditional check
if binary in ALL_KEYS:
before convertinginput_data
to nested dictionaries is a good practice. It ensures that the conversion only happens for supported binaries, which can prevent potential errors with unsupported binaries. This change enhances the robustness of the code.src/quacc/recipes/espresso/dos.py (4)
- 2-2: The module documentation clearly states the purpose of the
dos.py
file, focusing on performing DOS calculations using Quantum ESPRESSO. This clarity helps users understand the scope and functionality of the module.- 1-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [52-86]
The
test_bands_job_with_fermi
function is well-implemented, covering the setup and execution of a bands job with Fermi surface calculations. It's important to ensure comprehensive test coverage, including scenarios where the Fermi surface calculation might fail or produce unexpected results.
- 1-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [52-86]
The
test_bands_job_with_fermi
function is well-implemented, covering the setup and execution of a bands job with Fermi surface calculations. It's important to ensure comprehensive test coverage, including scenarios where the Fermi surface calculation might fail or produce unexpected results.
- 1-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [52-86]
The
test_bands_job_with_fermi
function is well-implemented, covering the setup and execution of a bands job with Fermi surface calculations. It's important to ensure comprehensive test coverage, including scenarios where the Fermi surface calculation might fail or produce unexpected results.src/quacc/recipes/espresso/bands.py (4)
- 35-143: The
bands_job
function provides a comprehensive approach to performing bands and Fermi surface calculations, with flexibility to include post-processing and Fermi surface calculations based on user input. It's important to ensure that the function handles all possible error scenarios gracefully, especially when dealing with external binaries likepw.x
,bands.x
, andfs.x
.- 146-209: The
bands_pw_job
function is well-implemented, providing a mechanism to perform a basic bands calculation usingpw.x
. The optional generation of a high symmetry k-path using the Latmer-Munro approach is a valuable feature. Ensure that the function properly handles cases where the generation of the k-path might fail or produce unexpected results.- 212-258: The
bands_pp_job
function correctly sets up and executes a post-processing calculation usingbands.x
. This function is crucial for re-ordering bands and computing band-related properties. It's important to validate that the function correctly handles the input and output directories, especially in cases where the previous calculation's output might not be as expected.- 261-308: The
fermi_surface_job
function is designed to retrieve the Fermi surface usingfs.x
, requiring a uniform unshifted k-point grid from a previous bands calculation. This function adds significant value to the module by enabling Fermi surface analysis. Ensure that the function includes checks for the prerequisites of the Fermi surface calculation to prevent user errors.docs/user/recipes/recipes_list.md (1)
- 204-208: The additions of Espresso-related jobs and a Fermi surface job to the recipes list are well-structured and align with the PR's objectives to expand computational capabilities. Ensure that the placeholders for documentation links
[quacc.recipes.espresso.bands.bands_job][]
are intended to be filled in later or are in the process of being completed to maintain the usefulness and navigability of the documentation.src/quacc/calculators/espresso/espresso.py (1)
- 356-368: The handling of unsupported binaries with a warning and adjustments to
input_data
is a thoughtful approach to maintain flexibility while informing the user about potential limitations. Ensure that this behavior is thoroughly tested with various unsupported binaries to confirm that the adjustments and warning mechanism work as intended without introducing unexpected behavior.src/quacc/settings.py (1)
- 179-179: The addition of
"fs": "fs.x"
to theESPRESSO_BINARIES
dictionary is a straightforward and necessary update to support Fermi surface calculations as mentioned in the PR objectives. This change is consistent with the naming conventions and structure of the existing entries in the dictionary. However, it's important to ensure that any related documentation, especially user guides or API documentation, is updated to include this new binary and its intended use case. This will help users understand how to leverage the new Fermi surface calculation capabilities introduced in this PR.
… espresso_band_flow
@Nekkrad --- Thank you again for your contribution, and apologies for how long this has taken to get merged! I refactored this a bit to reduce some unnecessary code. I also changed the I am done with the cleanup here. The only challenge is that the tests run for a very long time now. Is there anything you can do in the parameters to make things less expensive? |
@Andrew-S-Rosen No worries! I will take a look at it tomorrow. I can probably make them run very quickly, will let you know once it's done |
Thanks!! That would be a huge help :) Will merge ASAP afterwards! |
@Andrew-S-Rosen I added few keywords to the bands_pw_job and flow. These allow the users to have a little more flexibility if they decide to use make_bandpath. I also changed the parameters for the tests, hopefully they should run quicker. Let me know if they are still very slow and/or if there are other ONETEP or QE recipes that run too slowly |
@Nekkrad: Thanks a ton! The time was dropped for the most expensive run by nearly 2x, which was a big help. Here is the current timing:
I'm going to go ahead and merge this, but certainly if you can think of other words to reduce the timing, I would be greatly appreciative. At some point though, I will probably need to just split the Espresso tests into two separate parts either way. As for ONETEP, right now the test suite is only running the mocked tests because the "real" ONETEP execution would happen on the Princeton HPC machines, and I haven't yet successfully compiled ONETEP. Eventually... Setting this to auto-merge! |
Summary of Changes
Checklist
main
.black
,isort
, andruff
as described in the style guide.Summary by CodeRabbit
quacc
library with recipes for conducting bands and Fermi surface calculations.quacc
recipes to include new Espresso bands-related and Fermi surface jobs.