Conversation
cgrambow
added
Arkane
Status: Ready for Review
There was a problem hiding this comment.
Code looks great, I have no comments, added a minor question.
Re which representation to use in the database, it might not be a bad idea to use the non-standardized form. Since the data depends on the software used, it sounds reasonable to also replace it with the name the software used. But I'm OK with any choice.
|
|
||
| with open(os.path.join(settings['database.directory'], 'quantum_corrections', 'lot_constraints.yml')) as f: | ||
| METHODS_THAT_REQUIRE_SOFTWARE = yaml.safe_load(f)['METHODS_THAT_REQUIRE_SOFTWARE'] | ||
| with open(os.path.join(settings['database.directory'], 'quantum_corrections', 'lot_constraints.yml')) as _f: |
There was a problem hiding this comment.
what's the difference between f and _f?
There was a problem hiding this comment.
I use f in a couple of the methods later on, so I wanted to avoid having a module-level variable with the same name.
Codecov Report
@@ Coverage Diff @@
## main #1967 +/- ##
==========================================
- Coverage 48.54% 46.62% -1.93%
==========================================
Files 110 88 -22
Lines 30812 23132 -7680
Branches 8054 6008 -2046
==========================================
- Hits 14959 10786 -4173
+ Misses 14329 11196 -3133
+ Partials 1524 1150 -374
... and 74 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
|
@oscarwumit would something like this be helpful or hurtful for ARC? |
|
Changing base to |
|
Conflicts were simple to resolve - @amarkpayne @oscarwumit @alongd @cgrambow please check this PR after tests run and see where we stand. |
Store LevelOfTheory attributes the same way that they are given when initializing a LevelOfTheory object, but use the standardized representations when checking equality and using objects as dictionary keys (hashing).
The string representations of level of theory objects used in the database might differ from those used in AE and BAC classes. However, write_to_database has to edit the database file as a text file instead of treating it as a Python module, so make sure to match the correct string representation of the level of theory in the database. UPDATE: upon rebasing (3 years later), it looks to me (rwest) like this commit is redundant, so by the time I resolved the conflicts, as far as I can tell, there is nothing left. But I'm leaving the message here. The original commit was eabdee0
|
This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days. |
github-actions
bot
added
the
abandoned
4 participants
Motivation or Problem
LevelOfTheorystandardizes all of its attributes upon initialization, which can cause issues in software like ARC that must differentiate between different string representations. See ReactionMechanismGenerator/ARC#399.Description of Changes
Implement
LevelOfTheory._std_tuple, which stores the standardized representation of the object, and no longer modify the public attributes which are provided as arguments during instance creation.LevelOfTheory._std_tupleis used for equality comparisons and hashing, thus retaining the functionality of using level of theory objects as dictionary keys and standard representations of levels of theory.Testing
Expanded unit tests to reflect the changes.
Additional Questions
Currently, the database (
quantum_corrections/data.py) uses the standard string representations for all of the attributes. Do we want to change those to use "nonstandard" strings for better readability? Not necessary for this PR and doesn't affect any Arkane functionality.