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

[WIP] Initial sketch of MagneticOrderingsMaker #39

Closed
wants to merge 3 commits into from

Conversation

mkhorton
Copy link
Member

@mkhorton mkhorton commented Dec 21, 2021

Summary

Not ready for review yet, will keep PR open as I develop.

This PR will be for a magnetic orderings workflow, ported from MagneticOrderingsWf:

https://github.com/hackingmaterials/atomate/blob/main/atomate/vasp/workflows/base/magnetism.py#L109

This is largely following ElasticRelaxMaker as an example.

Additional dependencies introduced (if any)

  • List all new dependencies needed and justify why. While adding dependencies that bring
    significantly useful functionality is perfectly fine, adding ones that add trivial
    functionality, e.g., to use one single easily implementable function, is frowned upon.
    Provide a justification why that dependency is needed. Especially frowned upon are
    circular dependencies.

TODO (if any)

  • Write minimally-working flow
  • Add test case (likely NiO, small enumeration)
  • Resolve TODOs in code (likely cases where I still need to understand best practices here)
  • Ensure atomate.common.schema.MagnetismDocument is sufficient general, and integrates nicely with emmet builders

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP] in the pull request
title.

Before a pull request can be merged, the following items must be checked:

  • 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
    pycodestyle, followed by flake8.
  • Docstrings have been added in theNumpy docstring format.
    Run pydocstyle on your code.
  • Type annotations are highly encouraged. Run mypy to
    type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more
efficient if you already fix most errors prior to submitting the PR. It is highly
recommended that you use the pre-commit hook provided in the repository. Simply run
pre-commit install and a check will be run prior to allowing commits.

@mkhorton mkhorton marked this pull request as draft December 21, 2021 21:29
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2022

Codecov Report

Merging #39 (fdfd7da) into main (ce31ec0) will decrease coverage by 1.49%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   70.65%   69.15%   -1.50%     
==========================================
  Files          46       49       +3     
  Lines        3973     4069      +96     
  Branches      622      633      +11     
==========================================
+ Hits         2807     2814       +7     
- Misses       1006     1094      +88     
- Partials      160      161       +1     
Impacted Files Coverage Δ
src/atomate2/common/schemas/magnetism.py 0.00% <0.00%> (ø)
src/atomate2/vasp/flows/magnetism.py 0.00% <0.00%> (ø)
src/atomate2/vasp/jobs/magnetism.py 0.00% <0.00%> (ø)
src/atomate2/vasp/files.py 92.18% <0.00%> (-0.67%) ⬇️

@mattmcdermott
Copy link
Member

@mkhorton @utf
I will happily take this over if you guys are okay with it! I want to run some magnetic ordering workflows right now and figured it was worth finalizing Matt's PR :)

Anything I should know in particular about this? Do you want it to be a common workflow independent of the DFT code or is it okay living in the vasp module?

@mkhorton
Copy link
Member Author

That would be very kind! No preference on location, it's probably possible to generalise without much difficulty, but would be easier to write in a VASP-specific way.

I don't think this affects any of @guymoore13's effort, but nevertheless tagging him just in case, since he's custodian of the other (future) magnetic workflows.

@mkhorton
Copy link
Member Author

Btw I'm happy to review/offer comments if you do take this on.

@mattmcdermott
Copy link
Member

Happy to help! You have a great draft to work with.

I could probably make this a common workflow without too much difficulty.

And I appreciate the help with reviewing!

@Zhuoying
Copy link
Contributor

@mattmcdermott has taken over this PR in PR #432 with comments reviewed by @mkhorton, so close it here.

@Zhuoying Zhuoying closed this Oct 17, 2023
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.

4 participants