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

Add modification to aims input to match atomate2 magnetic order script #3878

Merged
merged 41 commits into from
Sep 7, 2024

Conversation

tpurcell90
Copy link
Contributor

@tpurcell90 tpurcell90 commented Jun 12, 2024

How it deals with spins is not through magmoms, but FHI-aims uses magmoms exclusively

Summary

Modifying FHI-aims inputs to work with the magnetism workflows in atomate2

Todos

If this is work in progress, what else needs to be done?

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

How it deals with spins is not through magmoms, but FHI-aims uses
magmoms exclusively
@tpurcell90 tpurcell90 changed the title Add modification to aims input to match atomate2 magnetic order script Draft: Add modification to aims input to match atomate2 magnetic order script Jun 14, 2024
Standardize the inputs handeling of magmoms and spin in the sepecies
1) Parse mulliken inpformation
2) Better standardize relaxation final chunk
     Done because mulliken analysis only happens for the final
     structure and after the normal parsing line is stated
@tpurcell90 tpurcell90 requested a review from janosh as a code owner June 19, 2024 17:14
@tpurcell90 tpurcell90 changed the title Draft: Add modification to aims input to match atomate2 magnetic order script Add modification to aims input to match atomate2 magnetic order script Jun 19, 2024
@tpurcell90
Copy link
Contributor Author

ready for review

@mkhorton
Copy link
Member

Thanks for the PR @tpurcell90, can you expand on:

How it deals with spins is not through magmoms, but FHI-aims uses magmoms exclusively

I'm not sure I understand, apologies.

@tpurcell90
Copy link
Contributor Author

Can this be merged in?

@tpurcell90
Copy link
Contributor Author

@mkhorton @shyuep @janosh Can this be merged in once I resolve the conflicts?

@janosh
Copy link
Member

janosh commented Sep 2, 2024

i'd be happy to merge but i think @mkhorton is much more knowledgeable here. so would be good to get his approval. any remaining concerns @mkhorton?

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality magmoms Magnetism related ecosystem Concerning the larger pymatgen ecosystem labels Sep 2, 2024
@tpurcell90
Copy link
Contributor Author

Test passed locally will check if they pass here ready for review

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nitpicks

src/pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/inputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/parsers.py Outdated Show resolved Hide resolved
src/pymatgen/io/aims/parsers.py Outdated Show resolved Hide resolved
tpurcell90 and others added 7 commits September 6, 2024 12:19
Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Signed-off-by: Thomas Purcell <purcellt@arizona.edu>
Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Signed-off-by: Thomas Purcell <purcellt@arizona.edu>
Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Signed-off-by: Thomas Purcell <purcellt@arizona.edu>
Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Signed-off-by: Thomas Purcell <purcellt@arizona.edu>
Remove stray \
replace split(",spin") with re.sub(",spin=F", "") with label
Evaluatlion of array error
@tpurcell90
Copy link
Contributor Author

I should have incorporated all of the suggestions from @janosh

@mkhorton mkhorton merged commit 05c2e99 into materialsproject:master Sep 7, 2024
43 checks passed
@mkhorton
Copy link
Member

mkhorton commented Sep 7, 2024

Thanks for your patience during review @tpurcell90, appreciate your PR!

@janosh
Copy link
Member

janosh commented Sep 7, 2024

missed a few typos in my review. fixed in 149e115.

@tpurcell90 I noticed that it might make more sense to turn AimsSpeciesFile into a dataclass instead of implementing total ordering manually:

def __eq__(self, other: object) -> bool:
"""True if two species are equal."""
if not isinstance(other, AimsSpeciesFile):
return NotImplemented
return self.data == other.data
def __lt__(self, other: object) -> bool:
"""True if self is less than other."""
if not isinstance(other, AimsSpeciesFile):
return NotImplemented
return self.data < other.data
def __le__(self, other: object) -> bool:
"""True if self is less than or equal to other."""
if not isinstance(other, AimsSpeciesFile):
return NotImplemented
return self.data <= other.data
def __gt__(self, other: object) -> bool:
"""True if self is greater than other."""
if not isinstance(other, AimsSpeciesFile):
return NotImplemented
return self.data > other.data
def __ge__(self, other: object) -> bool:
"""True if self is greater than or equal to other."""
if not isinstance(other, AimsSpeciesFile):
return NotImplemented
return self.data >= other.data

@tpurcell90 tpurcell90 mentioned this pull request Sep 8, 2024
4 tasks
@tpurcell90
Copy link
Contributor Author

I made it a dataclass in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem Concerning the larger pymatgen ecosystem enhancement A new feature or improvement to an existing one io Input/output functionality magmoms Magnetism related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants