Fix ORCA hessian decode failing on missing CARTESIAN GRADIENT block - #47
Conversation
Analytic Hessian jobs don't print CARTESIAN GRADIENT block in ORCA stdout. Split gradient parsing into non-required parse_gradient_hessian for Hessian calctype to prevent decode() raising when the gradient block is missing.
_trj.xyz doesn't carry charge/multiplicity info, so Structure.open_multi was silently defaulting to (0, 1) regardless of electronic state.
|
Found a second bug with optimizations always returning |
|
@TroyNSmith Will take a look at this Sunday/Monday :) Thank you for this! Also, good news--I won the grant for Atoms for Humanity so starting September I'll be working on this full time to really take these packages to the next level :) |
|
@coltonbh Thanks! That's so exciting, congratulations on the award! I can't wait to see how these packages evolve over the next year! |
|
@TroyNSmith apologies! Will look at this tomorrow/Wednesday! Conference is taking all my time! Thanks :) |
coltonbh
left a comment
There was a problem hiding this comment.
Hi Troy! This looks really good. Thanks for the test coverage :)
Let's just update the test to assert the correctness of the Hessian parsing too and not just the presence of the value, as that adds some robustness to our test suite.
Then I'll merge and release a new version!
| computed_props = decode("orca", "hessian", stdout=stdout, directory=orca_dir) | ||
|
|
||
| assert computed_props.gradient is None | ||
| assert computed_props.hessian is not None |
There was a problem hiding this comment.
Let's update this to check for correctness too and not just that the value exists :)
Fix minor issue with parsing ORCA Hessians where only a single column exists in the row
|
@coltonbh I apologize for the delay in addressing your comment... finally settling down after 2 long months of travel 🥲 Fixed On another note: I don't remember applying this fix, but apparently there was an issue with parsing ORCA hessians in the case where the final row of the Hessian only contains one column... that fix is now packaged into this PR. It seems like it was a regex issue (aka one-line fix). I added a test case to On another another note: I ran On another another another note: I looked at CONTRIBUTING.md for guidance on your pre-commit workflow (linting, formatting, testing, ...), but I didn't see any. If this exists, then I apologize for missing them--if not, would you consider adding a section ( |
|
@TroyNSmith Thanks for the thoughts on updating CONTRIBUTING.md. Yes--this all needs to happen! AFH will kickoff full time Sept 8 and I'll be bringing all of these packages into a much nicer state for community contributions. Please continue to make requests that will facilitate your workflow and I'll get them incorporated :) For now, let's get this fix shipped! |
|
@TroyNSmith Released in v0.11.2 🥳 |
Analytic Hessian jobs don't print
CARTESIAN GRADIENTblock in ORCA stdout. Split gradient parsing into non-required parse_gradient_hessian for Hessian calctype to prevent decode() raising when the gradient block is missing:Original error:
@coltonbh, @avcopan may be out of town so would not be able to review. Your call on whether to review yourself--no rush either way.