-
Notifications
You must be signed in to change notification settings - Fork 79
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
PwParser
: Fix trajectory verification for fixed_coords
#794
Conversation
41f8692
to
c0f69e2
Compare
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 the implementation is ok, it is quite elegant, but it needs validation and some comments explaining it.
7ecb160
to
ef95a54
Compare
@sphuber have added some more explanation regarding the Note that since the |
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 , overall the changes look good, just some minor things. Would be good to give this a go for real if possible.
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.
Pre-approving but if possible would still be good to verify this works in real example with @Sokseiha
I'm still testing + adding tests! And actually found some bugs, so good thing too ^^ |
There, now she's ready for another review. I've added tests for both the input file generation and the parsing ( |
5304b03
to
57dbaf2
Compare
@sphuber still found a flaw in the current implementation (i.e. the parsing expected |
@sphuber: As a side note: what should the "best practises" for validator methods be? i.e. do you agree with the following:
|
I agree on both points. I also add validators as public class methods or static methods |
3075940
to
c42b8e9
Compare
@sphuber this has been tested in the field by @AriannaCantarella. If you could give one final sign off on this one I'll then rebase #818 and we can merge that as well. |
c42b8e9
to
3d934f8
Compare
For `relax` and `vc-relax` calculations, the `PwParser` does an extra check on the `TrajectoryData` output to see if the structure has been properly converged. However, when comparing the final forces with the threshold, it does not consider that some site coordinates may have been fixed with the `fixed_coords` setting. Here we extract the `fixed_coords` setting from the `PwCalculation` inputs, and pass it to the `verify_convergence_trajectory` function. This has been adapted to ignore forces for fixed coordinates by setting the corresponding forces to zero before comparing them with the force threshold.
3d934f8
to
d9ef731
Compare
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 good to go!
Fixes #792
For
relax
andvc-relax
calculations, thePwParser
does an extracheck on the
TrajectoryData
output to see if the structure has beenproperly converged. However, when comparing the final forces with the
threshold, it does not consider that some site coordinates may have been
fixed with the
fixed_coords
setting.Here we extract the
fixed_coords
setting from thePwCalculation
inputs, and pass it to the
verify_convergence_trajectory
function.This has been adapted to ignore forces for fixed coordinates by setting
the corresponding forces to zero before comparing them with the force
threshold.