search

materialsproject / atomate2

atomate2 is a library of computational materials science workflows
https://materialsproject.github.io/atomate2/
Other
167 stars 96 forks source link

Fix `prev_dir` behavior in input set generator of `MPGGAStaticMaker` #996

Closed Andrew-S-Rosen closed 1 month ago

Andrew-S-Rosen commented 1 month ago

Closes #995.

Andrew-S-Rosen commented 1 month ago

@janosh: I was also wondering about that --- I had to dig through classes to see what value inherit_incar was being set to just to be sure. I will add inherit_incar=False to the other sets here that way when people make a new one, they will be more likely to ensure that this is still the case regardless of what happens upstream.

Definitely agree about the regression test. I wish I could be more help on this. If nobody is able to chip in, I'll see if I can carve out some time in the next week or two.

Andrew-S-Rosen commented 1 month ago

@janosh: Under-promise and over-delivering over here --- I added a regression test for the prev_dir behavior with MPGGAStaticMaker.

However, what is concerning is that there does not appear to be any pre-existing tests for MPGGAStaticMaker in test/vasp/jobs/test_mp.py so we may only be testing that Maker indirectly. That's for another PR though.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.01%. Comparing base (96b2b82) to head (428ebd1). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #996 +/- ## ========================================== + Coverage 4.38% 76.01% +71.62% ========================================== Files 174 174 Lines 12671 12690 +19 Branches 1890 1892 +2 ========================================== + Hits 556 9646 +9090 + Misses 12081 2506 -9575 - Partials 34 538 +504 ``` | [Files with missing lines](https://app.codecov.io/gh/materialsproject/atomate2/pull/996?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject) | Coverage Δ | | |---|---|---| | [src/atomate2/vasp/jobs/mp.py](https://app.codecov.io/gh/materialsproject/atomate2/pull/996?src=pr&el=tree&filepath=src%2Fatomate2%2Fvasp%2Fjobs%2Fmp.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject#diff-c3JjL2F0b21hdGUyL3Zhc3Avam9icy9tcC5weQ==) | `92.59% <ø> (+92.59%)` | :arrow_up: | ... and [156 files with indirect coverage changes](https://app.codecov.io/gh/materialsproject/atomate2/pull/996/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=materialsproject)
Andrew-S-Rosen commented 1 month ago

Would it be possible to get a bugfix release after this PR? I want to reduce potential issues with users trying to reproduce MP settings and getting conflicting results, especially given how hard it is to track down...

utf commented 1 month ago

v0.0.17 should be released shortly @Andrew-S-Rosen. Thanks for reporting this.