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 support to LOBSTER >=5.0 #4065

Merged
merged 39 commits into from
Oct 17, 2024

Conversation

naik-aakash
Copy link
Contributor

@naik-aakash naik-aakash commented Sep 13, 2024

Summary

This PR aims to update the lobster.io.inputs and lobster.io.outputs modules to support LOBSTER v5. There are some changes in v5.1, mainly with the format of output files. Also, new output files are generated from fragment orbital analysis. A few new keywords have also been added.

Todos

  • add new keywords from lobster >= v5 to the inputs module
  • update ICOHPLIST parser
  • update GROSSPOP parser
  • update Lobsterout parser
  • Add parser for POLARIZATION.lobster lobster file (LOBSTER >=5.0 generates this)
  • Add parser for BWDF.lobster lobster file
  • Add support for COXXCAR.LCFO.lobster, ICOXXLIST.LCFO.lobster, GROSSPOP.LCFO.lobster file parsing
  • Add support for DOSCAR.LCFO.lobster, CHARGE.LCFO.lobster file parsing
  • Add attributes to lobsterout parser for MOFE generated files
  • Add tests

@JaGeo
Copy link
Member

JaGeo commented Sep 13, 2024

@naik-aakash thanks! I was about to start doing it myself 🙈

@janosh janosh added enhancement A new feature or improvement to an existing one io Input/output functionality lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction) labels Sep 13, 2024
@JaGeo
Copy link
Member

JaGeo commented Sep 15, 2024

I think we also don't have a parser for the polarization file 😅. But maybe, this can be done in a subsequent pull request

@JaGeo
Copy link
Member

JaGeo commented Sep 25, 2024

@naik-aakash should we maybe get a part of this merged as a bug fix and do the other things later?

@QuantumChemist
Copy link
Contributor

@naik-aakash should we maybe get a part of this merged as a bug fix and do the other things later?

I was in contact with the LOBSTER developers and they will release a hotfix this week for a bug that prevents the LCFO anaylsis currently. I would suggest to wait with merging until then.

@naik-aakash
Copy link
Contributor Author

naik-aakash commented Sep 25, 2024

@naik-aakash should we maybe get a part of this merged as a bug fix and do the other things later?

Hi @JaGeo , we can do this. I can add test for this updates in next 1 or 2 days.

Also, a bug was found in the new MOFE feature. Thus, I could not generate test files or write parsers for outputs for that specific analysis. In anycase a new version for LOBSTER is expected soon.

@naik-aakash naik-aakash marked this pull request as ready for review October 2, 2024 09:54
@naik-aakash naik-aakash changed the title [WIP] Add support to LOBSTER >=5.0 Add support to LOBSTER >=5.0 Oct 2, 2024
@naik-aakash
Copy link
Contributor Author

Hi @JaGeo, this PR can now be reviewed. I am happy to address any issues that are spotted or needs some change.

Copy link
Member

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

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

I left some thoughts. Overall, nice work 😃

src/pymatgen/io/lobster/outputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/lobster/outputs.py Outdated Show resolved Hide resolved
src/pymatgen/io/lobster/outputs.py Outdated Show resolved Hide resolved
@naik-aakash
Copy link
Contributor Author

Hi @JaGeo, I have now addressed all the review comments where possible. For the Cohpcar and Doscar parsers, I do not see any other way to extract file types from the data within the file itself, so I have left them as they are, and it seems to work for now.

@JaGeo
Copy link
Member

JaGeo commented Oct 6, 2024

So far, we mostly let users choose the filetype (eg Icohps, Icoops). I would do the same here.

@naik-aakash
Copy link
Contributor Author

So far, we mostly let users choose the filetype (eg Icohps, Icoops). I would do the same here.

Oh, okay. This way sounds much simpler. I have now added the is_lcfo arg to the relevant parsers with the docstrings.

Copy link
Member

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

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

This looks great @naik-aakash ! Thank you!

@shyuep @mkhorton this would be ready to merge from my side. Please let us know if we should change anything else. Thank you. The test that fails currently is related to matplotlib and windows and I don't think this has anything to do with our pull request.

@naik-aakash
Copy link
Contributor Author

naik-aakash commented Oct 17, 2024

Hi @shyuep , now all tests also pass after last merge. If there are no specific comments on the changes, PR is ready to be merged.

@shyuep shyuep merged commit 9933f45 into materialsproject:master Oct 17, 2024
43 checks passed
@shyuep
Copy link
Member

shyuep commented Oct 17, 2024

Great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one io Input/output functionality lobster Lobster package (Local Orbital Basis Suite Towards Electronic-Structure Reconstruction)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants