-
Notifications
You must be signed in to change notification settings - Fork 82
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
PwBaseWorkChain
: improve restart from parent_folder
#722
Merged
+279
−92
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
8f95aa2
`PwBaseWorkChain`: improve restart from `parent_folder`
mbercx 8a0a6e9
Add validator for the restart parameters
mbercx 7063aa2
Replace `restart_calc` logic with `set_restart_type`
mbercx a9fc164
Apply reviewer suggestions
mbercx 762edc9
Switch to raising errors for incorrect `nscf` or `bands` restart inputs
mbercx a700e91
Fix PwBaseWorkChain tests
mbercx ca2ec28
Add tests for restart validation
mbercx c7b897c
apply excellent reviewer suggestions
mbercx d4eedce
clean up the tests a bit from my silly comments
mbercx 3aa820c
Raise warning instead of error for nscf/bands and `restart_mode==from…
mbercx 2c08469
And then fix the test, of course
mbercx a47f5df
Merge branch 'develop' into fix/721/base-restart
mbercx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys, I wanted just to point out that it could be useful to have also
FROM_FILES
(or something like that), so that one can restart from the density and wave functions. I know that 'full' is meant to do so, but it will also read the atomic positions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first inclination would be to say that restarting from the wave functions, it should be pretty fast for QE to recalculate the charge density. But if I remember correctly, QE doesn't actually do this, instead just plugging in the wave functions but for the potential calculated from the atomic charge density, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be like this. Two case scenario would be:
But I do agree at the end it is not such a difference. I was just wondering, since the new implementation is so cool (great job!!! :D ), it would be quite easy to just add that one more, just in case one needs it.
Or do you think that the "experienced" user can still always tweak the inputs if that is the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, @bastonero! Note that the restart types are only used for setting the restarts after an error has been handled using the
set_restart_type
method. So if a user wants to restart from a previous calculation, the correct inputs have to be provided, not the restart type.Happy to implement another restart type (not sure about the name,
FROM_FILES
seems a bit too general, maybeFROM_CHARGE_AND_WFC
?). Do you think there is already a current error handler where this restart type would be used though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, I see, thanks! For the moment, actually, I still don't have an example of handler where it would be useful.
FROM_CHARGE_AND_WFC
sounds good anyway.