Add energy contributions - #36
Conversation
dev-zero
left a comment
dev-zero
left a comment
There was a problem hiding this comment.
Some minor remarks, but agreed, that's something useful to have, thanks a lot!
| if not match: | ||
| if match: | ||
| energies["total force_eval"] = match["value"] | ||
| spans += match.spans(0) |
There was a problem hiding this comment.
| spans += match.spans(0) | |
| spans += [match.spans(0)] |
There was a problem hiding this comment.
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?
|
|
||
| INNER_SCF_START_RE = re.compile(r"^\s+ SCF\ WAVEFUNCTION\ OPTIMIZATION", re.VERBOSE | re.MULTILINE) | ||
| INNER_SCF_END_RE = re.compile( | ||
| INNER_SCF_CONV_RE = re.compile( |
There was a problem hiding this comment.
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.
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.
Co-authored-by: Tiziano Müller <tm@dev-zero.ch>
Co-authored-by: Tiziano Müller <tm@dev-zero.ch>
Co-authored-by: Tiziano Müller <tm@dev-zero.ch>
Co-authored-by: Tiziano Müller <tm@dev-zero.ch>
…wards incompatibility.
|
Thank you for the swift review and feedback. Except maybe for the format of the spans I believe it is ready to be merged. |
2 participants
Hi Tiziano,
I thought it would be nice to parse in addition to the force consistent energy also the uncorrected total energy and other energy contributions, e.g. to check the impact of dispersion corrections etc. As for the levelparser I added the energies to the InnerSCF block.
I didn't add new tests but adjusted the existing ones to also check the new properties.
Please let me know what you think.
All the best,
Holger