Improved TS energy checks #690
Conversation
alongd
force-pushed
the
ts_check
branch
2 times, most recently
from
4126581 to
6efb2b1
Compare
Codecov Report
@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 73.24% 73.31% +0.07%
==========================================
Files 99 99
Lines 26582 26609 +27
Branches 5558 5563 +5
==========================================
+ Hits 19469 19508 +39
+ Misses 5771 5758 -13
- Partials 1342 1343 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| return False | ||
| return True | ||
| return False | ||
| return rxn_copy |
There was a problem hiding this comment.
It is not clear from the name and use of this function that it should return a reaction object. The name contains compute, so it should either return a computed result, or populate an object property. Could you elaborate and consider renaming?
There was a problem hiding this comment.
I added a comment in the docstring. It's indeed unclear from the name, but the use does show it, also the type hint and now the docstring.
| @@ -2431,20 +2429,22 @@ def check_rxn_e0_by_spc(self, label: str): | |||
| """ | |||
| for rxn in self.rxn_list: | |||
| labels = rxn.reactants + rxn.products + [rxn.ts_label] | |||
| if label in labels and not rxn.ts_species.ts_checks['E0'] \ | |||
| if label in labels and rxn.ts_species.ts_checks['E0'] is None \ | |||
There was a problem hiding this comment.
Not None is equivalent to True, no need to change this line.
There was a problem hiding this comment.
Right, this is just semantics, not logic. I think the current version is more explicit.
arc/scheduler.py
Outdated
| and all([(species_has_sp(output_dict) and species_has_freq(output_dict)) | ||
| or self.species_dict[spc_label].yml_path is not None | ||
| and all([(species_has_sp(output_dict, self.species_dict[spc_label].yml_path) | ||
| and species_has_freq(output_dict, self.species_dict[spc_label].yml_path)) |
There was a problem hiding this comment.
good catch, the original code would have caused issues and wasn't readable!
|
Thanks, @kfir4444, I addressed the comments with fixup commits, let me know if you have additional comments or whether I can go ahead and squash |
Now it does not run the check, and returns the reaction copy with populated E0 values
Assuming that if the Arkane YAML path is not None, the the species has sp/geo/freq
Calling the checks from a centralized function
Refactoring ts_check functions
Tests added