-
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
PwRelaxWorkChain
: Revisit work chain logic
#952
base: main
Are you sure you want to change the base?
Conversation
@sphuber some initial notes/questions/decisions:
|
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 @mbercx . I think this is looking good. Left some comments.
Should the initial relaxation be run by default? Currently it is, and if we don't make it the default I'm wondering how to allow the user to enable it while also being able to provide a protocol for this step.
Is it the default? If the base_init_relax
namespace is not defined at all, it won't be run. Do you maybe mean the default when using get_builder_from_protocol
? If it works really well for most real use-cases, I think it makes sense to actually use it by default. We can add a note in the documentation how to skip it, which should not be complicated.
Re the documenation: I was planning to describe the work chain Purpose and Logic in the Topics section. What do you think?
Yeah, makes perfect sense.
base_init_relax: | ||
kpoints_distance: 0.3 | ||
meta_parameters: | ||
conv_thr_per_atom: 1.e-6 | ||
etot_conv_thr_per_atom: 1.e-3 | ||
pw: | ||
parameters: | ||
CONTROL: | ||
calculation: scf | ||
forc_conv_thr: 0.5e-2 | ||
CELL: | ||
press_conv_thr: 2 |
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.
Is there an origin for these numbers other than your thumb? :D Not that I have any better ones per se, but just curious if these are already tested in someway
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.
} | ||
} | ||
new_kpts_mesh, _ = create_kpoints_from_distance(**inputs_create_kpoints).get_kpoints_mesh() | ||
if tuple(input_kpts_mesh) != tuple(new_kpts_mesh): |
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.
EDIT: just saw your point in the PR description. Will leave this here because it contains some more thoughts on the exact criterion.
Should we maybe here check only that the new grid is not denser? If the new required grid is less dense, then there is no point in rerunning is there? The criterion for kpoints_distance
that a minimum density is guaranteed in order to guarantee a minimum precision.
One tricky thing I guess might be if you have non isometric meshes, such as (2, 3, 1)
and (3, 2, 1)
. Which one is denser? I guess there you would have to compute the actual distance between kpoints in 1/A for both both meshes in each direction and as soon as just one is larger for the new structure, we conclude we have to rerun.
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'm not sure how common that case last case is, but it's a fair point. Maybe we can accept when the new mesh is denser along every direction. This would not accept your last use case, but I think that's fine, and simpler than really calculating the density.
fae0e0d
to
3ed7e91
Compare
3ed7e91
to
aa6eb9d
Compare
de02473
to
b21fc70
Compare
b21fc70
to
1dfdade
Compare
Fixes #733
Fixes #705
Will write full commit message when all changes are agreed upon.