-
Notifications
You must be signed in to change notification settings - Fork 103
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 magnetic orderings workflow #432
Add magnetic orderings workflow #432
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
==========================================
- Coverage 74.94% 74.71% -0.24%
==========================================
Files 136 140 +4
Lines 10513 10792 +279
Branches 1643 1673 +30
==========================================
+ Hits 7879 8063 +184
- Misses 2143 2229 +86
- Partials 491 500 +9
|
Amazing! I will review but don't expect major comments. |
Thanks! Yes feedback especially on schemas would be useful -- I did base them on your previous sketches though. |
FYI: I moved the builder into Since this is the first builder in |
Approach for |
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.
Great PR! Approved by me, although noting I'm not the maintainer here :) I added some comments mostly for context/gotchas.
for group in grouped_tasks: | ||
group_parent_structure = task["metadata"]["parent_structure"] | ||
|
||
# parent structure should really be exactly identical (from same workflow) |
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 like this function. I wonder if we might benefit from the same code but in an exact_match()
utility function somewhere instead, it seems like it might be useful more generally.
"as defined in pymatgen.analysis.magnetism.analyzer." | ||
), | ||
) | ||
magmoms: list[float] = Field(None, description="Magnetic moments of the structure.") |
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.
Not a request to modify, but adding a note for reference: it would be useful to round magnetic moments somewhat, since getting a primitive structure (with respect to the magnetic moments, i.e. without losing magnetic order) will not work as intended unless equivalent moments are exactly equal. Typically, the noise in the moment is sufficient such that this is not the case. I did not get around to adding this however.
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.
Should we enable round_magmoms=True
in the CollinearMagneticStructureAnalyzer
?
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.
Yes, I'd do that. Eventually the underlying algorithm in round_magmoms
should be updated however. Simple rounding (e.g. np.round
) will sometimes round two similar magmoms in opposite directions. I tried instead doing a KDE of the magmoms with a suitable Gaussian width and taking the peak positions, but this felt like it was over-complicating things!
relax_output: MagneticOrderingRelaxation | None = Field( | ||
None, description="Relaxation output, if relaxation performed." | ||
) | ||
energy_diff_relax_static: str | float | None = Field( |
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.
Might be worth noting this field was only really present for the purpose of benchmarking (i.e., is the static calculation required).
Since I wrote the original workflow, I have since concluded that the static step should--at minimum--have a much smaller EDIFF, however I have not benchmarked this. The original benchmark results speak for themselves, however the difference in energies between orderings is sometimes very very small, such that I would have more confidence if we had a tighter tolerance.
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.
This is an important (and still unresolved) point. Right now, the default static calculations in the workflow are made with the VASP StaticMaker
, which I believe is EDIFF=1e-5
.
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 default StaticMaker
(EDIFF=1e-5) is looser than TightRelaxMaker
(ENCUT=700, EDIFFG=1e-7). Maybe we could use that parameter and this energy_diff_relax_static
term to benchmark and see if we need to improve the EDIFF in StaticMaker
.
) | ||
|
||
|
||
class MagneticOrderingsBuilder(MagneticOrderingsBuilderBase): |
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.
It's really nice to see how few additions were required to make this work with VASP. I wonder if this is a pattern that we should use elsewhere in atomate2? (If it's not already?)
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 it's a nice pattern, although it was tricky to figure out. I hope that this addition will be a nice example for future workflows :)
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.
Looks nice and concise! We can follow this good example to migrate the current ElasticBuilder
under vasp
to common
.
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 @Zhuoying, agreed :)
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.
This would be great! I think we could then easily connect the forcefields as well 😃
src/atomate2/vasp/flows/magnetism.py
Outdated
relaxations will be skipped (i.e., only static calculations). | ||
default_magmoms : dict | None | ||
Optional default mapping of magnetic elements to their initial magnetic moments | ||
in µB. Generally these are chosen to be high-spin, since they can relax to a |
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.
One thing that might be useful to verify is the default spin on Co. Since the original workflow was written, the pymatgen default changed to low-spin Co, since unlike other elements, Co seemed happier to be trapped in a high-spin configuration even if less stable. Strictly, I think Co should probably be run twice with both configurations.
Btw I haven't forgotten about this; just a little preoccupied. Need to address a bug still and write tests! |
@utf I just implemented your suggestion in #466. This workflow now contains a postprocessing job that shares much of the doc-constructing code used in the builder. In addition to feedback about the postprocessing implementation, your feedback on how this is implemented within I'll be working on tests in the meantime and will let you know when they're ready :) |
Test for magnetic ordering workflow has been added (using the developer guide). Since writing this code, @mcgalcode and I have also run the workflow and builder on 50+ structures, and it seems stable overall. @utf Is there anything else I should add? It would be great if you could review Matt H's comments above as well; one discussion point is whether we should make the static jobs have a smaller EDIFF by default (e.g., 1e-6 or 1e-7). Instead, we could leave it the way it is (i.e., default I see now that the new test is failing on GitHub Actions because |
This seems more than sufficient to approve. Would recommend leaving the EDIFF to the current atomate2 static default unless benchmarking suggests a tighter tolerance is required (although I agree it might help). Re. |
@Zhuoying please can you take a look at this PR and the comments. @mattmcdermott once @Zhuoying has had a look over, I will do a final pass. Regarding enumlib, I think we can follow @mkhorton's advice and try and install the one on conda forge. Longer term, it would be great if we could replace enumlib with a Python only package like bsym |
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.
Nice structured workflow. Can be used as a good example for new developers when they need a standardized postprocessing. The remaining issue might be:
(i) Is the default incar settings (EDIFF=1e-5) in StaticMaker sufficient (may need some benchmark evidence).
(ii) Double-check Co default spin (default changed to low-spin in pymatgen)
(iii) Pydantic v2 update (if any)
Thanks @mattmcdermott for the excellent PR.
) | ||
|
||
|
||
class MagneticOrderingsBuilder(MagneticOrderingsBuilderBase): |
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.
Looks nice and concise! We can follow this good example to migrate the current ElasticBuilder
under vasp
to common
.
relax_output: MagneticOrderingRelaxation | None = Field( | ||
None, description="Relaxation output, if relaxation performed." | ||
) | ||
energy_diff_relax_static: str | float | None = Field( |
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 default StaticMaker
(EDIFF=1e-5) is looser than TightRelaxMaker
(ENCUT=700, EDIFFG=1e-7). Maybe we could use that parameter and this energy_diff_relax_static
term to benchmark and see if we need to improve the EDIFF in StaticMaker
.
Hi @Zhuoying, thanks for the nice feedback and code review; this is very helpful!
I do think EDIFF for the static calc should be stricter by default, per @mkhorton's recommendation. So far, I have run relax+static calcs on 285 different orderings (from 26 structures) with the current defaults (EDIFF=1e-5). I made a histogram of all their (normalized) The mean difference is -0.0016 eV/atom (i.e. the static calculation tends to be that much lower in energy than the relax), but there are some outliers! I don't have a good sense on what this means with regards to picking an EDIFF, but I imagine this supports the idea that we should be stricter... Thoughts?
While the default Co is now low-spin, the This means that right now the default for the magnetic orderings workflow will be to initialize Co to high-spin. @mkhorton recommended running both low-spin and high-spin. Is this something we should implement in pymatgen or implement here?
Will look into! |
atomate2/src/atomate2/common/jobs/phonons.py Line 355 in 2843d04
|
Just to shortly add to my last comment, @mattmcdermott : I think one would need to compare the energies resulting from two static runs. comparing the optimization with a static run could lead to wrong conclusions as the volume might change during the optimization which has an influence on the basis set quality and therefore the energy from the optimization run. |
@mattmcdermott Thanks for providing the benchmarking histogram! It gives some quantitative evidence on whether we should choose a more strict EDIFF. Two follow-up comments: (ii) @utf The discussions and replies to my review are done. Now it is ready for your final review. |
@mattmcdermott This is a personal judgement call. Co has historically been tricky, and seems more likely to get stuck in a high-spin configuration than other elements. The decision to switch to a low-spin by default was simply because it seemed more common (i.e., what would be the most sensible default value?) but of course plenty of high-spin Co materials exist, and this is all anecdotal. The most exhaustive option would be to consider multiple states for all elements, but this would likely be quite inefficient. For the purposes of this workflow, if you can predict the oxidation state ahead of time, that's helpful, and the default magnetic moment can be set differently according to that. I would not recommend any changes to this workflow at this time, there isn't enough evidence either way (that I know of). At the end of the day, porting this workflow to atomate2 at all is super helpful, and it doesn't mean further improvements can't be made later. Modifying the workflow/enumerator to allow multiple spin states per element would be an option, but I think this might be an awkward change. Perhaps the best option is simply giving the user a warning if they're calculating a material including Co and suggest they attempt both? |
Thanks all for your help. I will update the PR today, and then we should be good for final review by @utf |
I added support for |
If I can chime in here. I just posted a bug issue in pmg which is affecting the atomate MagWF. materialsproject/pymatgen#3431 It could be useful to test if that bug affects the current MagWF in atomate2 as well. |
This looks fantastic. Thanks so much @mattmcdermott. Once tests pass I'm happy to merge. |
882fa27
to
fe751cf
Compare
@utf the magnetic orderings workflow is now implemented only in common, with defaults for VASP and a note about ensuring compatible Note I bumped the The failing tests are from other modules. No rush to pull but let me know if you are envisioning any other changes! |
This is fantastic. Thanks very much! The tests are failing because of a recent change in emmet. I can merge this once materialsproject/emmet#974 is merged and a new version released. |
Just merged, it should be released soon. |
@utf I am merging as now all tests pass and you said it is good to merge as soon as the emmet version is fixed (only codecov slightly decreased now) |
* Initial sketch of `MagneticOrderingsMaker` * Add schema to match original atomate `MagneticOrderingsToDb` format * move magnetism jobs to common * move magnetism flows to common * clean up magnetic ordering documentation/typing * make "prev calc dir" a code-dependent argument name for static maker * clean up typing, imports, docstrings * begin porting over magnetic workflow analysis code / schemas * fix jobflow logic for magnetic enumerator flow * rework for loop for orderings * migrate magnetic orderings wf to builder format * Improve naming * remove output storing * add magnetic ordering builder implementation * remove unused imports in magnetism jobs * fix bug in task creation in magnetism builder * update magnetic ordering job documentation * make sure name "magnetic orderings" is consistent * make sure name "magnetic orderings" is consistent pt. 2 * move magnetic orderings builder into common * ensure copying of magmoms between relax and static; linting * fix copy magmom * fix kwarg location (mistake previously) * add final False to copy vasp magmoms * debug test of magmoms * fix bug in magmoms * delete unused line * move magnetic ordering & symmetry comparison logic to common * linting * linting pt 2 * add from_tasks() support; extract logic to common * add and fix postprocessing job * linting * fix error in structure matching in builder It's actually dangerous to match by lattice/site matrices; sometimes the structures are not identically defined despite being the same structure * add test for magnetic ordering workflow * Add warning if relax_maker not provided; stricter EDIFF settings * Add enumlib install and Cobalt warning * move Co warning * add Optional to magnetism schemas * ignore enumlib's tests * make testing directory explicit * regenerate test data for mag_ordering workflow * fix broken test (due to new EDIFF=1e-7 data) * remove the patch fix we had for prev_dir now that atomate2 is updated * fix breaking python 3.8 test * move imports and fix typing problem * try making tests dir not explicit * change dir in github action to move enumlib outside testing scope * update @mattmcdermott contributor info * attempt to fix coverage CI bug * monty decoding for magnetism builder * fully implement magnetic_orderings workflow into common * add warning about TaskDoc * bump emmet-core version * clean up magnetic ordering wf files * bump emmet-core again --------- Co-authored-by: Matthew Horton <[email protected]> Co-authored-by: J. George <[email protected]>
* Initial sketch of `MagneticOrderingsMaker` * Add schema to match original atomate `MagneticOrderingsToDb` format * move magnetism jobs to common * move magnetism flows to common * clean up magnetic ordering documentation/typing * make "prev calc dir" a code-dependent argument name for static maker * clean up typing, imports, docstrings * begin porting over magnetic workflow analysis code / schemas * fix jobflow logic for magnetic enumerator flow * rework for loop for orderings * migrate magnetic orderings wf to builder format * Improve naming * remove output storing * add magnetic ordering builder implementation * remove unused imports in magnetism jobs * fix bug in task creation in magnetism builder * update magnetic ordering job documentation * make sure name "magnetic orderings" is consistent * make sure name "magnetic orderings" is consistent pt. 2 * move magnetic orderings builder into common * ensure copying of magmoms between relax and static; linting * fix copy magmom * fix kwarg location (mistake previously) * add final False to copy vasp magmoms * debug test of magmoms * fix bug in magmoms * delete unused line * move magnetic ordering & symmetry comparison logic to common * linting * linting pt 2 * add from_tasks() support; extract logic to common * add and fix postprocessing job * linting * fix error in structure matching in builder It's actually dangerous to match by lattice/site matrices; sometimes the structures are not identically defined despite being the same structure * add test for magnetic ordering workflow * Add warning if relax_maker not provided; stricter EDIFF settings * Add enumlib install and Cobalt warning * move Co warning * add Optional to magnetism schemas * ignore enumlib's tests * make testing directory explicit * regenerate test data for mag_ordering workflow * fix broken test (due to new EDIFF=1e-7 data) * remove the patch fix we had for prev_dir now that atomate2 is updated * fix breaking python 3.8 test * move imports and fix typing problem * try making tests dir not explicit * change dir in github action to move enumlib outside testing scope * update @mattmcdermott contributor info * attempt to fix coverage CI bug * monty decoding for magnetism builder * fully implement magnetic_orderings workflow into common * add warning about TaskDoc * bump emmet-core version * clean up magnetic ordering wf files * bump emmet-core again --------- Co-authored-by: Matthew Horton <[email protected]> Co-authored-by: J. George <[email protected]>
Summary
This PR includes the addition of a flow and builder for collinear magnetic ordering enumeration/calculations. This flow is based on the original
MagneticOrderingsWF
in atomate1. This extends the first sketch by @mkhorton in #39.The final postprocessing step is included as a job and as a builder.
Most of the workflow logic is written within
common
. In this PR I have implemented everything with VASP.TODO (if any)
MagOrderingTransformation
by enforcing implicit-zero magnetic moments inSpaceGroupAnalyzer
pymatgen#3070Checklist
The easiest way to handle this is to run the following in the correct sequence on
your local machine. Start with running black on your new code. This will
automatically reformat your code to PEP8 conventions and removes most issues. Then run
ruff.
Run ruff on your code.
type check your code.