Element count from conformer #1651
Conversation
alongd
force-pushed
the
element_count_from_conf
branch
3 times, most recently
from
8196764
to
7423a38
Compare
Codecov Report
@@ Coverage Diff @@
## master #1651 +/- ##
=========================================
+ Coverage 41.68% 41.7% +0.01%
=========================================
Files 176 176
Lines 29342 29342
Branches 6050 6050
=========================================
+ Hits 12232 12237 +5
+ Misses 16240 16236 -4
+ Partials 870 869 -1
Continue to review full report at Codecov.
|
alongd
force-pushed
the
element_count_from_conf
branch
from
7423a38
to
50fb17f
Compare
| is_species_in_dict = False | ||
| if os.path.isfile(spec_dict_path): | ||
| with open(spec_dict_path, 'r') as f: | ||
| # check whether the species dictionary contains this species, in which case do not re-append |
| else: | ||
| element_counts[symbol] = 1 | ||
| return element_counts | ||
|
|
There was a problem hiding this comment.
It feels like this method might do better as a part of the Conformer class.
There was a problem hiding this comment.
I looked at it, but this method uses symbol_by_number() from Arkane common.py, I wouldn't like to make Arkane a dependency of conformer, not to add the symbol_by_number() function to conformer. I say let's leave it as is. Would you agree?
There was a problem hiding this comment.
When looking at the usages of symbol_by_number and the corresponding get_element_mass, they all relate in someway to the conformer object's attributes (some of the uses involve converting C to 12 in the Log classes, which is then just converted back in arkane). Ideally, I would think almost any method manipulating the atom's XYZ coordinates and their corresponding masses should be within the Conformer class, since this complies with the principle of encapsulation in object oriented programming.
Though honestly, it would require some restructuring, and I am fine merging it in, we already have many other poor uses of object oriented programing in the code already.
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main(testRunner=unittest.TextTestRunner(verbosity=2)) |
There was a problem hiding this comment.
Even if we decide to move the conformer counting method to Conformer.pyx, having this unittest would still be super useful (so no need to eliminate the whole thing).
alongd
force-pushed
the
element_count_from_conf
branch
2 times, most recently
from
33d4fb4
to
ccb8162
Compare
|
@goldmanm, I added another (minor) commit for consistent naming of |
For now only tests the element_count_from_conformer() method, should be expanded.
THe Species.props['element_counts'] is only used in Arkane. renamed elementCounts to element_counts to be consistent with prior changes
alongd
force-pushed
the
element_count_from_conf
branch
from
b763b62
to
af10c4f
Compare
Motivation or Problem
Previously thermo blocks in Chemkin files outputted by Arkane did not have an element count if the molecular structure wasn't specified by the user.
Description of Changes
Now we use the xyz information Arkane has for each species (specifically the species.conformer.number attribute) to determine the element count.
Additional minor modifications:
open()close()withwith open()Testing
Added a thermoTest for the new element_count_from_conformer() method.
Reviewer Tips
Try running a ThermoJob without specifying a structure and look at the Chemkin output