Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Element count from conformer #1651

Merged
merged 4 commits into from
Jul 23, 2019
Merged

Element count from conformer #1651

merged 4 commits into from
Jul 23, 2019

Conversation

alongd
Copy link
Member

@alongd alongd commented Jul 12, 2019

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:

  • replaced file open() close() with with open()
  • don't append a species adjList dictionary to the species dictionary file if the species is already there

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

@alongd alongd requested a review from goldmanm July 12, 2019 13:24
@alongd alongd self-assigned this Jul 12, 2019
@alongd alongd mentioned this pull request Jul 12, 2019
@alongd alongd force-pushed the element_count_from_conf branch 3 times, most recently from 8196764 to 7423a38 Compare July 12, 2019 19:26
@codecov
Copy link

codecov bot commented Jul 12, 2019

Codecov Report

Merging #1651 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
rmgpy/data/kinetics/family.py 52.9% <0%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68367e8...af10c4f. Read the comment docs.

@alongd alongd force-pushed the element_count_from_conf branch from 7423a38 to 50fb17f Compare July 12, 2019 20:06
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice check

else:
element_counts[symbol] = 1
return element_counts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this method might do better as a part of the Conformer class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alongd force-pushed the element_count_from_conf branch 2 times, most recently from 33d4fb4 to ccb8162 Compare July 20, 2019 20:36
goldmanm
goldmanm previously approved these changes Jul 21, 2019
@alongd
Copy link
Member Author

alongd commented Jul 21, 2019

@goldmanm, I added another (minor) commit for consistent naming of element_counts in Species.props
It's set in Arkane statmech.py and isn't used in RMG
With this, the PR should now be good to go

goldmanm
goldmanm previously approved these changes Jul 21, 2019
alongd added 4 commits July 21, 2019 19:44
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 alongd force-pushed the element_count_from_conf branch from b763b62 to af10c4f Compare July 21, 2019 23:45
@goldmanm goldmanm self-requested a review July 23, 2019 01:49
@goldmanm goldmanm merged commit 492d3f6 into master Jul 23, 2019
@goldmanm goldmanm deleted the element_count_from_conf branch July 23, 2019 01:49
@mliu49 mliu49 mentioned this pull request Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants