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

Support two different makers for DoubleRelaxMaker #32

Merged
merged 8 commits into from
Jan 28, 2022
Merged

Support two different makers for DoubleRelaxMaker #32

merged 8 commits into from
Jan 28, 2022

Conversation

Andrew-S-Rosen
Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen commented Dec 10, 2021

Summary

Addresses #30, wherein the DoubleRelaxMaker flow can now take in two separate BaseVaspMaker args in case the user wants the first or second relax job to have different settings. A common use-case could be to set user_kpoint_settings in the first relax maker to be half the usual value. Ideally, there'd be an optional parameter called half_kpts_first_relax that could do this automatically, but I had some trouble figuring out the best way to modify the k-point density of the first job in a clean way that accounts for KSPACING vs. the other k-point generation schemes.

Edit: In hindsight, I guess the user could always have just defined the flow first and then edited the desired job.maker.input_set_generator afterwards... hmm...

@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as draft December 10, 2021 07:17
@utf
Copy link
Member

utf commented Dec 23, 2021

I think this is a reasonable approach to take. Am I ok to merge?

@utf
Copy link
Member

utf commented Dec 23, 2021

I actually think relax_maker1 and relax_maker2 could be better than relax_maker and relax_maker2. Just for style and consistency.

@Andrew-S-Rosen
Copy link
Member Author

Got it! I'll make that consistent throughout (eg in the tests too).

@Andrew-S-Rosen
Copy link
Member Author

Also, sorry about the hacky commits here. I'm coding in quick bursts at the airport 😅 I'll get it sorted out and ready to merge soon though.

@Andrew-S-Rosen Andrew-S-Rosen marked this pull request as ready for review January 23, 2022 06:27
Copy link
Member

@utf utf left a comment

Choose a reason for hiding this comment

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

Sorry, you might still be editing but noticed a couple of things.

src/atomate2/vasp/flows/amset.py Outdated Show resolved Hide resolved
src/atomate2/vasp/flows/core.py Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Jan 26, 2022

@utf, yup I'm still editing. I pushed the change because I have some questions. The tests fail due to the LREAL assertion. Do you know why this might be? I'm having trouble pinpoint it.

Definitely a (useful) learning process!

Edit: And don't worry, I'll squash all the commits before the PR. I mainly wanted to prematurely push here so you could see where I was struggling.

tests/vasp/flows/test_core.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Member Author

Andrew-S-Rosen commented Jan 28, 2022

@utf, okay the tests finally pass now. However, could I ask you to take care of the symlinking? I am on Windows and am worried that it's going to wreak havoc on things. The only files that need to be kept as an actual file are the .../inputs/INCAR in the Si_double_relax_swaps subdirectories. The rest can be symlinks.

@codecov-commenter
Copy link

Codecov Report

Merging #32 (03dec13) into main (fe5d8a6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   70.39%   70.38%   -0.02%     
==========================================
  Files          45       45              
  Lines        3804     3809       +5     
  Branches      576      576              
==========================================
+ Hits         2678     2681       +3     
- Misses        979      981       +2     
  Partials      147      147              
Impacted Files Coverage Δ
src/atomate2/vasp/flows/amset.py 0.00% <ø> (ø)
src/atomate2/vasp/flows/elastic.py 94.73% <ø> (ø)
src/atomate2/vasp/flows/elph.py 84.09% <ø> (ø)
src/atomate2/vasp/flows/core.py 93.91% <100.00%> (+0.27%) ⬆️
src/atomate2/utils/path.py 88.23% <0.00%> (-11.77%) ⬇️

@utf
Copy link
Member

utf commented Jan 28, 2022

Fantastic, thank you!

@utf utf merged commit d21bdac into materialsproject:main Jan 28, 2022
@Andrew-S-Rosen Andrew-S-Rosen deleted the rosen-doublerelax branch January 28, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants