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 tests for the New Vasp input sets #3576

Merged
merged 22 commits into from
Jan 31, 2024
Merged

Conversation

Zhuoying
Copy link
Contributor

@Zhuoying Zhuoying commented Jan 24, 2024

Summary

Major changes:

Todos

  • Add more MD and NED-related tests
  • Fix lints

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

@janosh janosh added tests Issues with or changes to the pymatgen test suite io Input/output functionality vasp Vienna Ab initio Simulation Package labels Jan 24, 2024
@Zhuoying
Copy link
Contributor Author

Zhuoying commented Jan 24, 2024

Hi @utf, I have a quick question here: is there any particular reason for skipping ENCUT in INCAR (set as None) in MITMDSet and MPMDSet?

"ENCUT": None,

"ENCUT": None,

The default settings in MVLNPTMDSet look more reasonable to me if someone just wants to manually run an MD wo workflow and grab some snapshots from a single run.

updates["ENCUT"] = max(enmax) * 1.5

@utf
Copy link
Member

utf commented Jan 25, 2024

@Zhuoying, I didn't change the behaviour of the input sets. I just updated the implementation. E.g., for the two input sets you mentioned, see the old implementation where ENCUT was also removed from the input set.

I would highlight that the point of these tests is to ensure that the output of the new input set implementation is the same as the output of the old implementations.

@Zhuoying Zhuoying changed the title [WIP] Add tests for the New Vasp input sets Add tests for the New Vasp input sets Jan 26, 2024
@janosh
Copy link
Member

janosh commented Jan 29, 2024

thanks @Zhuoying! more tests are always appreciated! is this ready for review?

@Zhuoying
Copy link
Contributor Author

@janosh Yeah, it is ready to review now. Pytest code coverage for pymatgen/io/vasp/sets.py improved from 93% to 95%.
The two failing checks are not related to this PR. Please let me know if you have any comments.
For other io.vasp modules (inputs, outputs, optics), I may submit new PRs for them later if needed.
Thanks.

Name                           Stmts   Miss  Cover
--------------------------------------------------
pymatgen/io/vasp/__init__.py       3      0   100%
pymatgen/io/vasp/help.py          35     35     0%
pymatgen/io/vasp/inputs.py      1071    109    90%
pymatgen/io/vasp/optics.py       186     18    90%
pymatgen/io/vasp/outputs.py     2350    211    91%
pymatgen/io/vasp/sets.py        1204     65    95%
--------------------------------------------------
TOTAL                           4849    438    91%

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.

thanks a lot @Zhuoying! 👍 i feel safer already with this new test coverage (no joke) 😄

Copy link
Member

Choose a reason for hiding this comment

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

general hint: no need to check coverage files into version control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint. I guess I did it unconsciously...

@janosh janosh enabled auto-merge (squash) January 31, 2024 08:38
@JaGeo
Copy link
Member

JaGeo commented Jan 31, 2024

Yes, thanks a lot for these efforts!

@janosh janosh merged commit 7dd059c into materialsproject:master Jan 31, 2024
22 checks passed
@utf
Copy link
Member

utf commented Jan 31, 2024

This is great, thanks very much @Zhuoying

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Input/output functionality tests Issues with or changes to the pymatgen test suite vasp Vienna Ab initio Simulation Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants