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.
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
Feature/relax and bands wc #254
base: master
Are you sure you want to change the base?
Feature/relax and bands wc #254
Changes from 4 commits
046b97c
d5a7179
67f5906
137ff8b
53c042f
28bd5b4
324d9b5
67be159
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This should use
expose_inputs
. If the namespace is fully optional, you setpopulate_defaults = False
: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.
Instead of doing this manually, I would write a recursive function and call it:
Then in the
define
method, you simply call the method: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.
Wouldn't rely on this through a default because then a user can run anything else. We should always set this no? So just set it in the workchain step when preparing the inputs
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.
I thought this was against the idea to fully expose the inputs of the sub-processes, but ok, this makes sense. And also avoids the problem of my last comments above.
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.
Was wondering if we should have a slightly different outline:
I think this is a bit more explicit and clear as to what will happen.
This would make the relax optional, which is still useful, because the user can then compute the bands for an already optimized structure but be sure that the bands are computed in exactly the same way as if it were to include relaxation.
I would also call the second relax run an SCF, because that is what it is, even though we are using the
RelaxWorkflow
but that is merely because we decided that this is the way an SCF is run.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.
I think I am sensing a conceptual difference here. In my mind the user would NOT have the possibility to decide whether to run an extra scf or not. To determine the presence of an extra step is only the fact that seekpath was used (and in the future maybe the decision to us a different code for bands compared to relax). The user could only select some inputs for this step.
Your idea only gives the additional flexibility to run an extra scf even when it is not necessary (when seekpath is not used) and I believe it is a valid use case.
However I have strong doubts about your other suggestion to make the relaxation optional
This can be done already setting the
relax_type
to None in therelax
input. So it is not needed. I do not see any other case when this is useful and the price to pay is very high. In fact I believe that making both therelax
step and thescf
step optional is difficult to support. How do we understand that the user wants only thescf
and no the relaxation? If we makepopulate_defaults = False
andrequired = False
for both relaxation and scf, then we have to implement additional logic that checks that one of the two is set, and that the one selected sets at least thestructure
,protocol
,engines
. Basically it makes useless to expose the inputs since None of the properties of the inputs port can be used, they must be implemented all over again. It seems to me an additional complication that has no motivation to exist.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.
I agree that making both optional adds more complexity. But I don't understand your current design. If I understand correctly, in
prepare_bands
you check whether explicit kpoints were specified in thebands
option, in which case you don't rerun the relax workflow as scf. But the first relax workflow and seekpath are always run. So it is practically guaranteed that the structure changes, even if the user setsRelaxType.NONE
. Even if the relax doesn't change the structure, it is possible that seekpath still normalizes the structure, which will cause the k-points to longer be commensurate.I guess what I am saying is that we should make the relax+seekpath optional, or otherwise allowing to directly specify k-points doesn't make any sense. Or we just require relax+seekpath+scf always and just don't support the user specifying their own k-points. At least that would be consistent.
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.
Not exactly. The relaxation always run. The use of SeeKpath is optional. If you set a kpoints for bands in input, SeekPath is not used. This is a valid use case, for instance, I know the kpoints path along which I want to calculate the bands, and I also want a relaxation of just the atoms coordinates. Of course it does not make much sense to specify a kpoints path and also allow relaxation with variable cell, since you will not know the final cell shape. But the rest is perfectly fine.
When you DO NOT use Seekpath (i.e. you specify kpoints for bands in input), you do not need an extra scf. In fact, you pass to the bands calculation the
remote_folder
of the relaxation and this guarantees to copy the correct final structure and the density matrix / wave-functions. However, based to your suggestion, I understood that the possibility for the user to trigger an extra scf anyway, is a valid use case. For instance a user wants to do a final scf with "precise" protocol before calculating bands.To summarize, three options are available
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.
This is what was confusing me. Should we maybe have validation in there to prevent this? If you specify kpoints, then the relax cannot change the cell.
This is great, this makes things a lot clearer. I would propose to at least describe this in the docstring of the workchain. And then maybe adapt the outline to the following that makes it a lot clearer what is happening: