Add magnetic orderings workflow

[mattmcdermott]

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)

• [x] Finalize and merge PR into pymatgen for broken magnetic code https://github.com/materialsproject/pymatgen/pull/3070
• [x] Fix possible bugs related to orderings changing between static and relax
• [x] Explore moving even more code into common (especially builder code)
• [x] Add tests

Checklist

• [x] Code is in the standard Python style. 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.
• [x] Docstrings have been added in the Numpy docstring format. Run ruff on your code.
• [x] Type annotations are highly encouraged. Run mypy to type check your code.
• [x] Tests have been added for any new functionality or bug fixes.
• [ ] All linting and tests pass.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 69.89247% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 74.71%. Comparing base (29a5731) to head (e117f6f).

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     

Files	Coverage Δ
src/atomate2/common/schemas/magnetism.py	96.85% <96.85%> (ø)
src/atomate2/common/jobs/magnetism.py	89.13% <89.13%> (ø)
src/atomate2/common/flows/magnetism.py	77.50% <77.50%> (ø)
src/atomate2/common/builders/magnetism.py	0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

[mkhorton]

Amazing! I will review but don't expect major comments.

[mattmcdermott]

Thanks! Yes feedback especially on schemas would be useful -- I did base them on your previous sketches though.

[mattmcdermott]

FYI: I moved the builder into common. Of course, this opens up the possibility of the builder mixing outputs from different DFT codes; I addressed this by adding a property that is implemented in child classes. This property, _dft_code_query, is tacked onto the get_items query to ensure that only calcs from one DFT code are used.

Since this is the first builder in common, maybe we can standardize an approach like this for future common builders?

[JaGeo]

Approach for common sounds very good!

[mattmcdermott]

Btw I haven't forgotten about this; just a little preoccupied. Need to address a bug still and write tests!

[mattmcdermott]

Bug was addressed. See Issue #505 where AFM orderings were being destroyed between relax and static calcs. Fix was provided in PR #506.

Still need to write tests and then we should be about ready to merge!

[mattmcdermott]

@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 common would be very helpful, as this workflow might set the precedence for future additions to atomate2.

I'll be working on tests in the meantime and will let you know when they're ready :)

[mattmcdermott]

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 StaticMaker) and let the user change this if desired.

I see now that the new test is failing on GitHub Actions because enumlib is a required dependency to enumerate magnetic orderings. What do you think we should do here?

[mkhorton]

have also run the workflow and builder on 50+ structures

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. enumlib, it's an easy compile but it's not consistently versioned. The one on conda-forge does seem like it was last updated 9 months ago, so fairly recent--perhaps we could try installing this in CI?

[utf]

@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

[mattmcdermott]

Hi @Zhuoying, thanks for the nice feedback and code review; this is very helpful!

The remaining issue might be: (i) Is the default incar settings (EDIFF=1e-5) in StaticMaker sufficient (may need some benchmark evidence).

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) energy_diff_relax_static values.

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?

(ii) Double-check Co default spin (default changed to low-spin in pymatgen)

While the default Co is now low-spin, the MagneticStructureEnumerator still uses the older default magnetic moments by loading them from file here:

https://github.com/materialsproject/pymatgen/blob/cfd47add8958af795dabcfe1661cf8890ad56415/pymatgen/analysis/magnetism/default_magmoms.yaml#L1-L4

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?

(iii) Pydantic v2 update (if any)

Will look into!

[JaGeo]

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) energy_diff_relax_static values.

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?

1e-7 or at least 1e-6. 1e-7 is usually my default for manual phonon runs. I also implemented a phonon static run that could be used (will be moved from common to vasp in one of the open pull requests): https://github.com/materialsproject/atomate2/blob/2843d040c76f507ca38cede4cd3cf85a55ee3e52/src/atomate2/common/jobs/phonons.py#L355

[JaGeo]

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.

[Zhuoying]

@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: (i) EDIFF in static_maker: From the histogram, there are more than 10% of orderings (~30/285) Ediff >10 meV/atom. If the difference in energies between orderings is sometimes very very small (<5meV/atom), the current default (1e-5) needs to be modified to a smaller value (i.e., 1e-7) rather than leaving it for users to decide IMO.

(ii) relax_maker=None: I suppose when you have confidence in your input structure (well-relaxed), you can turn the relax_maker off. I have not come up with a situation where you can do this. -If magnetism for different orderings is set well and done with relaxation, it sounds like manually doing the first half of the workflow. If not and skip the relaxation step, I might be worried about the accuracy since magnetism does matter for the optimized structures. So if you have any idea when we can turn it off, please let me know. Otherwise, we might consider adding a warning for relax_maker=None and turning it on as the default?

@utf The discussions and replies to my review are done. Now it is ready for your final review.

[mkhorton]

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?

@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?

[mattmcdermott]

1. @Zhuoying and @JaGeo Thank you for the helpful discussion on EDIFF values. I agree with your assessment and will implement EDIFF=1e-7 as the new default.
2. @Zhuoying Regarding the optional relaxation -- note that the VASP-implemented workflow includes a RelaxMaker by default (it only defaults to None in the common flow). I will add a warning when None is supplied, though.
3. @mkhorton Providing the warning you described for Co-containing structures seems like the best solution for now.

Thanks all for your help. I will update the PR today, and then we should be good for final review by @utf

[mattmcdermott]

I added support for enumlib installation during testing with Github Actions per the commands used in the pymatgen github workflows. Adds about 30s to the testing run; hopefully this is okay!

[fraricci]

If I can chime in here. I just posted a bug issue in pmg which is affecting the atomate MagWF. https://github.com/materialsproject/pymatgen/issues/3431

It could be useful to test if that bug affects the current MagWF in atomate2 as well.

[utf]

This looks fantastic. Thanks so much @mattmcdermott. Once tests pass I'm happy to merge.

[mattmcdermott]

Thanks for the nice feedback @utf. All the tests are passing :)

The failure is a CI issue only (can't reproduce on my laptop). It seems related to calculating coverage. I'll figure this out today so we can merge the PR.

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/_pytest/main.py", line 271, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/_pytest/main.py", line 325, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pluggy/_hooks.py", line 493, in __call__
INTERNALERROR>     return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pluggy/_manager.py", line 115, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pluggy/_callers.py", line 130, in _multicall
INTERNALERROR>     teardown[0].send(outcome)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pytest_cov/plugin.py", line 298, in pytest_runtestloop
INTERNALERROR>     self.cov_controller.finish()
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pytest_cov/engine.py", line 44, in ensure_topdir_wrapper
INTERNALERROR>     return meth(self, *args, **kwargs)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/pytest_cov/engine.py", line 254, in finish
INTERNALERROR>     self.cov.combine()
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/coverage/control.py", line 832, in combine
INTERNALERROR>     combine_parallel_data(
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/coverage/data.py", line 176, in combine_parallel_data
INTERNALERROR>     data.update(new_data, aliases=aliases)
INTERNALERROR>   File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/coverage/sqldata.py", line 6[60](https://github.com/materialsproject/atomate2/actions/runs/6884676495/job/18727615337?pr=432#step:7:61), in update
INTERNALERROR>     raise DataError("Can't combine line data with arc data")
INTERNALERROR> coverage.exceptions.DataError: Can't combine line data with arc data

[Zhuoying]

It is glad that @utf gave a final approval. @mattmcdermott Thanks for all the hard work to make this PR look amazing! I just updated the branch and retriggered the tests, let's see if the CI issue still exists or not.

[mattmcdermott]

Thanks @Zhuoying. Now getting some failures on the tests for forcefields flows?

[Zhuoying]

@mattmcdermott Yeah, the failures seem not related with this PR. Let me ping @JaGeo, do you have any thoughts on it?

[JaGeo]

@janosh merged something new related to forcefields yesterday (https://github.com/materialsproject/atomate2/pull/611) where there were issues with other forcefield outputs. I fear it might be related.

[janosh]

I suspect (could be wrong) matgl is the culprit here. I just tried and failed to publish a new release (which may or may not fix this) over at https://github.com/materialsvirtuallab/matgl/releases/tag/v0.8.6. It's also affecting @esoteric-ephemera in https://github.com/materialsproject/emmet/pull/892 with a clearer error message (assuming it's the same issue here).

FAILED emmet-core/tests/test_ml.py::test_ml_doc[M3GNet-MP-2021.2.8-PES-prop_kwargs1] - ValueError: Bad serialized model or bad model name. It is possible that you have an older model cached. Please clear your cache by running python -c "import matgl; matgl.clear_cache()"

@shyuep Can someone in your group take a look?

[mattmcdermott]

@Zhuoying @utf tests are passing now. Codecov check is warning about the diff but seems to only apply to some lines in the builder (as far as I know, we do not have any tests for builders in atomate2).

[utf]

Thanks @mattmcdermott, I just had one thought about the schemas and VASP specific code needed.

[mattmcdermott]

@utf the magnetic orderings workflow is now implemented only in common, with defaults for VASP and a note about ensuring compatible TaskDoc implementation for alternate DFT codes.

Note I bumped the emmet-core version to include the TaskDoc change I made there (https://github.com/materialsproject/emmet/pull/945)

The failing tests are from other modules.

No rush to pull but let me know if you are envisioning any other changes!

[utf]

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.