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
Add energy contributions #36
base: develop
Are you sure you want to change the base?
Add energy contributions #36
Changes from 2 commits
13d82b4
60efdb4
49b9657
ede7500
a055b49
e74b640
48e3554
3591c12
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.
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 reversed this change since it was resulting in the cli tests to fail. Checking the output I believe that my original implementation was actually already giving the correct output?
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.
Please note that the blocks themselves can be used by other tools, changing the names will break them.
But if you think there is good reason to do so, please continue. The only thing I would then ask you is whether you can add a new entry in
CHANGELOG.md
for the next minor version (major.minor.patch) to make sure we don't forget to bump it on the next release for the backwards incompatible change.The reason I called it
SCF_END_RE
is because it's notoriously difficult to reliably determine whether the CP2K (SCF) finished properly. The idea was to assume that the presence of that line indicates a finished SCF, everything else not.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.
Sorry, I didn't take that into consideration. I just thought that as the energies are parsed further below in the output file and SCF_END_RE would refer to the end of the inner SCF cycle it would make sense to rename it.
However, having backwards compatibility is probably more important in this case, thus I reversed this change.