-
Notifications
You must be signed in to change notification settings - Fork 94
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
Transition to pymatgen VASP input sets #854
Conversation
@esoteric-ephemera thanks. I am currently not sure at all as I cannot forsee the sideeffects of the changes. |
I can foresee that the longer this is not done, the more side effects there will be. I have said it before - it is pointless to do coding in a different code base when it is something that is supposed to be another code base. Moving stuff later always creates more problems. |
@shyuep i am not opposing this. I just need to check if there might be something 😅. @esoteric-ephemera I guess we got our answer. lobster tests are breaking. 😬 |
Yeah I need to dig into this a bit - some of the tests here will fail because of the simultaneous update in pymatgen, but I need to see if that's why first |
src/atomate2/vasp/sets/base.py
Outdated
def from_directory(directory: str | Path, optional_files: dict = None) -> VaspInput: | ||
"""Load a set of VASP inputs from a directory. | ||
|
||
Note that only the standard INCAR, POSCAR, POTCAR and KPOINTS files are read |
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.
any reason to leave from_directory
and _get_nedos
here? could both be moved to pymatgen as well, which would allow us to remove the VaspInputGenerator
class from atomate2
entirely.
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.
Agreed this should be added to pymatgen.
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.
Moved to PMG
outcar: Outcar = None, | ||
) -> dict: | ||
@property | ||
def kpoints_updates(self) -> dict | Kpoints: |
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.
why dict | Kpoints
and not just dict
? if it's for parent class compat, prob the parent class needs to change
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.
Being able to return a Kpoints object directly is a nice feature! I think this might be in the pymatgen sets but worth checking.
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 strongly recommend homogeneous return type. just asking for trouble calling a Kpoints
method on kpoints_updates
that fails because it's a dict
.
i'd advise to always return either dict
or Kpoint
. given incar_updates
is dict
, would minimize user surprise to return dict
here as well
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.
Just noting that this is used in some places in pymatgen already:
- https://github.com/materialsproject/pymatgen/blob/442d740eb7794d360cb35f187e271b3debb7f047/pymatgen/io/vasp/sets.py#L2159
- https://github.com/materialsproject/pymatgen/blob/442d740eb7794d360cb35f187e271b3debb7f047/pymatgen/io/vasp/sets.py#L2218
I think having flexibility can be useful. That said, it wouldn't be much work to allow specifying a kpoints file within a dictionary format, but it does seem like extra boilerplate.
given incar_updates is dict, would minimize user surprise to return dict here as well
Incar
is already a dict so you could conceivably return Incar
too.
src/atomate2/vasp/sets/matpes.py
Outdated
_matpes_config_no_magmom["INCAR"].pop("MAGMOM") | ||
|
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.
MatPES set (@janosh and @shyuep): The MatPES magmoms were missing from the atomate2 set previously, but are present in their pymatgen set. For now, the atomate2 MatPES set removes the magmoms for consistency with previous versions, but that may have been a bug
that sounds like a bug! thanks for catching this! let's remove the .pop("MAGMOM")
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.
Done!
src/atomate2/vasp/files.py
Outdated
@@ -192,7 +192,7 @@ def write_vasp_input_set( | |||
) | |||
|
|||
if apply_incar_updates: | |||
vis.incar.update(SETTINGS.VASP_INCAR_UPDATES) | |||
vis["INCAR"].update(SETTINGS.VASP_INCAR_UPDATES) |
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 make sense to add
@property
def incar(self) -> Incar:
"""INCAR object."""
return self["INCAR"]
and co for POTCAR, POSCAR, ... to pymatgen
DictSet
so we can keep vis.incar
here
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.
Done on the pmg side
@esoteric-ephemera you could temp modify this step to install your pmg PR to test against it until both are ready to go atomate2/.github/workflows/testing.yml Lines 70 to 72 in aee4be8
- name: Install pymatgen from master if triggered by pymatgen repo dispatch
- if: github.event_name == 'repository_dispatch' && github.event.action == 'pymatgen-ci-trigger'
- run: pip install --upgrade 'git+https://github.com/materialsproject/pymatgen@${{ github.event.client_payload.pymatgen_ref }}'
+ run: pip install --upgrade 'git+https://github.com/materialsproject/pymatgen@refs/pull/854/head' |
Only the lobster flows (and not the jobs) seemed to be impacted by this change - these just needed a small modification to work correctly |
@esoteric-ephemera we are happy to look at it once the tests are passing. Why had the Lobster test file to be removed? Thanks in advance. |
I don't see a lobster test file removed; the |
@esoteric-ephemera I am referring to your last commit. Is it ready for us to take a look? Are the tests passing? |
Ok I see - not all of the lobster tests are run in a temporary directory, so if a test fails, there might be intermediate files written to disk. That happened and the Let me know if I should change that behavior for the tests The tests are passing on my side - I can't get pmg to build from git in the CI nor locally (some issues with c dependencies) but I'll look at that today |
Ah, I see! Thanks! We might want to modify the Lobster tests in a separate PR then. This will otherwise again lead to many changes not directly related to this PR. Thanks! I will test the workflows with the latest pymatgen PR tonight. |
@esoteric-ephemera I currently get very weird errors when using these versions of atomate2 and pymatgen to run the Lobster workflow. ("free(): invalid pointer"). @naik-aakash do you maybe have a chance to shortly install both this branch of atomate2 and the linked pymatgen branch and check the Lobster workflow? It might be something else off with my settings... |
@JaGeo which python did you build your env with? I'm using 3.11, but the cython libraries were reorganized between 3.10 and 3.11, that might lead to the C issues you're seeing (same reason the CI can't build an env for 3.11: as it recompiles the C libraries for python, and pymatgen explicitly |
@esoteric-ephemera 3.10, I think. Currently on a train (at least not stuck anymore) and cannot definitely confirm |
No worries @JaGeo. I rebuilt my python env with 3.10 and the tests in |
Thanks for pinging me for diagnosing @janosh. I believe materialsvirtuallab/monty#699 would fix this issue @esoteric-ephemera, please let me know if you have further comments, thanks! With: @deprecated(replacement=MatPESStaticSet, deadline=(2025, 1, 1))
@dataclass
class MatPesGGAStaticSetGenerator(MatPESStaticSet):
"""Class to generate MP-compatible VASP GGA static input sets."""
xc_functional: Literal["R2SCAN", "PBE", "PBE+U"] = "PBE"
auto_ismear: bool = False
auto_kspacing: bool = False We now have: from atomate2.vasp.sets.matpes import MatPesGGAStaticSetGenerator
generator = MatPesGGAStaticSetGenerator()
assert generator.xc_functional == "PBE"
assert not generator.auto_ismear
print(generator.as_dict()) Output:
|
Thanks @DanielYang59! That looks more like what I expect to see from the |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
==========================================
+ Coverage 74.94% 75.13% +0.19%
==========================================
Files 136 142 +6
Lines 10513 10614 +101
Branches 1643 1580 -63
==========================================
+ Hits 7879 7975 +96
- Misses 2143 2167 +24
+ Partials 491 472 -19
|
src/atomate2/common/files.py
Outdated
@@ -35,11 +35,11 @@ def copy_files( | |||
either "usernamehost" or just "host" in which case the username | |||
will be inferred from the current user. If ``None``, the local filesystem will | |||
be used as the source. | |||
include_files : None or list of (str or .Path) | |||
include_files : None | list[str | Path] |
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.
Is there a reason you changed the doc strings? I think the correct style is to use "or" in the string since it is human readable. E.g. see here https://numpydoc.readthedocs.io/en/latest/format.html#parameters
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 didn't make these changes, they're from here. But I'll revert them
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.
OK reverted!
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.
Hi @esoteric-ephemera just noticed one thing about the doc strings. After that, is this ready to merge or do we need to wait for a new monty release?
Just made the docstr changes. We could wait for monty, right now the deprecation warnings print something like:
which isn't very instructive because of the name used. Happy to change it if preferred |
Ok, once a new version of monty is released we can merge this. Thanks very much for your work getting this over the line. |
Sounds good and thanks! |
warning before: <string>:31: FutureWarning: __post_init__ is deprecated, and will be removed on 2025-01-01 Use MPRelaxSet in pymatgen.io.vasp.sets instead. warning after: <string>:31: FutureWarning: MPGGARelaxSetGenerator is deprecated, and will be removed on 2025-01-01 Use MPRelaxSet in pymatgen.io.vasp.sets instead.
|
Head branch was pushed to by a user without write access
Fantastic @janosh! Added a little fix for the Lobster schemas to work with the newer version of monty, pending your PR getting released |
Thanks @esoteric-ephemera this is fantastic. |
Summary
This PR moves all VASP input sets to the pymatgen input sets, and is dependent on pymatgen PR #3835 - now merged and released in
pymatgen==2024.5.31
. Merging this PR would close issues #844 and #724 (@Andrew-S-Rosen).This large change could have breaking consequences outside the VASP library. I've checked that the VASP and LOBSTER code bases are still working as intended, so no breaking changes are expected there. That we've already had incompatibilities between the pymatgen and atomate2 implementations of the same input sets really motivates why this change is needed.
Class duplication has been reduced significantly between pymatgen and atomate2.
atomate2.vasp.sets.base.VaspInputSet
has been removed, with any features that were not included in its pymatgen counterpart (pymatgen.io.vasp.sets.VaspInputSet
) moved to atomate2'sVaspInputGenerator
.VaspInputGenerator
now inherits from pymatgen'sDictSet
class. Some features formerly inVaspInputGenerator
are included inDictSet
per that PR. I've keptVaspInputGenerator
after some discussions with @utf and @janosh. This class still adds two methods not included inDictSet
, and which are not necessarily useful to include there. Theconfig_dict
of inherited classes are now set by their pymatgen equivalents when appropriate (e.g.,MPRelaxSet
for the MP GGA workflow). Some other changes to the file handling were needed because of this change.I need to update doc strings, but I wanted to wait on more cosmetic changes until we have consensus on code structure.Other changes:
pymatgen>=2024.6.4
(thanks @janosh!) andase>=3.23.0
(many many thanks @Andrew-S-Rosen!)Request for comments / feedback:
This PR is pretty much complete, but could use some input from various stakeholders:
For now, the atomate2 MatPES set removes the magmoms for consistency with previous versions, but that may have been a bugThis PR now corectly sets the magmoms for MatPES.VaspInputGenerator
by moving its features toRelaxSetGenerator
. Right now,RelaxSetGenerator
effectively aliasesVaspInputGenerator
. Any thoughts?