-
Notifications
You must be signed in to change notification settings - Fork 230
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
Determine the rotor symmetry number from the potential scan #1526
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1526 +/- ##
==========================================
- Coverage 42.08% 42.07% -0.02%
==========================================
Files 165 165
Lines 27824 27824
Branches 5668 5668
==========================================
- Hits 11711 11706 -5
- Misses 15328 15332 +4
- Partials 785 786 +1
Continue to review full report at Codecov.
|
55fad10
to
05f8196
Compare
530dbe6
to
bea31c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent PR, @alongd! There a couple of questions I have about how your code will function (see my review comments), and a couple suggestions for cleaning up the git history with this PR. I am also in the process of testing out the code, but I want to get these comments to you first.
rms_fourier = numpy.sqrt(numpy.sum((Vlist_fourier - Vlist) * (Vlist_fourier - Vlist))/ (len(Vlist) - 1)) / 4184. | ||
|
||
elif fit == 'best': | ||
rms_cosine = numpy.sqrt(numpy.sum((Vlist_cosine - v_list) * (Vlist_cosine - v_list)) / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't rename Vlist
to v_list
until the next commit though, right? If so, then if anyone were to reset their branch to this commit (without including your later commits) then this will throw an error. This isn't a big deal, but for consistent git history it might be worth moving the name change to this commit.
arkane/statmech.py
Outdated
# identify peaks and valleys, and determine worst resolutions in the scan | ||
if i != 0 and i != len(energies) - 1: | ||
# this is an intermediate point in the scan | ||
if e > energies[i - 1] and e > energies[i + 1]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A possible but optional thing you can do here would be to define variables for energies[i-1]
and energies[i+1]
since you reference these values a lot. The marginal code efficiency should be weighed against the readability of the code though, so this change is up to you.
valleys.append(e) | ||
# The number of peaks and valley must always be the same (what goes up must come down), if it isn't then there's | ||
# something seriously wrong with the scan | ||
if len(peaks) != len(valleys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens though if there is a peak or valley right on one of the end points? You only check the intermediate points a few lines above this. Will your code catch this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above we define peaks, valleys = list(), [energies[0]]
, so we take the first point into consideration, which is also represented by the last point (assuming a 360 deg scan). In a normal case both these points represent a valley, if it's a peak then this scan is problematic. This is checked elsewhere (only a warning is raised).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, sorry that I missed this
min_valley = min(valleys) | ||
max_valley = max(valleys) | ||
# Criterion 1: worst resolution | ||
if max_peak - min_peak > worst_peak_resolution: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my understanding of this is correct, you are assuming that for the symmetry number to be anything other than 1 that all of the peaks need to be the same size (within a tolerance). Is this true though? What if your scan had 4 peaks, with two larger peaks of height A
and two smaller peaks of height B
. If the peaks went ABAB, wouldn't this scan have a symmetry number of 2? Would your code catch this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, while this scenario would indeed have a symmetry number of two, we could not come up with a physical system that would have this property. Therefore, this technique should work in most if not all cases. No changes necessary.
@@ -270,24 +270,6 @@ def testTransitionStateStatmech(self): | |||
job.load() | |||
|
|||
|
|||
class TestStatmech(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this change to the next commit where you add statmechTest.py? The rest of this commit can probably be combined with one of your earlier syntax commits as well.
The ``symmetry`` parameter will usually equal either 1, 2 or 3. Below are examples of internal rotor scans with these | ||
commonly encountered symmetry numbers. First, ``symmetry = 3``: | ||
The ``symmetry`` parameter will usually equal either 1, 2 or 3. It could be determined automatically by Arkane | ||
(by simple not specifying it altogether), however it is always better to explicitly specify it if it is known. If it is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change by simple not specifying
to by simply not specifying
@@ -351,8 +351,10 @@ scan in the following format:: | |||
|
|||
The ``Energy`` can be in units of ``kJ/mol``, ``J/mol``, ``cal/mol``, ``kcal/mol``, ``cm^-1`` or ``hartree``. | |||
|
|||
The ``symmetry`` parameter will usually equal either 1, 2 or 3. Below are examples of internal rotor scans with these | |||
commonly encountered symmetry numbers. First, ``symmetry = 3``: | |||
The ``symmetry`` parameter will usually equal either 1, 2 or 3. It could be determined automatically by Arkane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way in .rst files to add a section of text from another file? That why, future updates to this in arkane/input.rst will automatically update the same section in input_pdep.rst. I am not sure if this is possible, but I will do some digging to find out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I don't know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears to be possible with the .. include ::
statement, but it involves moving the text to another file. I am not sure if it is worth doing then, as it will clutter the documentation source files a little bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it as is for now, I think we should change the pdep doc of Arkane to only include pdep stuff and avoid this repetition.
Also renamed Vlist as v_list
@amarkpayne, I addressed the comments, let me know what your thoughts are on the ABAB case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good! I was also able to run the Arkane examples, and your technique got the symmetry right every time. Take a quick look at my final comments, then squash the fix-up commits and this is ready to be merged.
valleys.append(e) | ||
# The number of peaks and valley must always be the same (what goes up must come down), if it isn't then there's | ||
# something seriously wrong with the scan | ||
if len(peaks) != len(valleys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, sorry that I missed this
min_valley = min(valleys) | ||
max_valley = max(valleys) | ||
# Criterion 1: worst resolution | ||
if max_peak - min_peak > worst_peak_resolution: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, while this scenario would indeed have a symmetry number of two, we could not come up with a physical system that would have this property. Therefore, this technique should work in most if not all cases. No changes necessary.
arkane/statmechTest.py
Outdated
|
||
import unittest | ||
import os | ||
import shutil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IDE suggests that this import statement is not needed, and I only see it in this file once. Can it be deleted?
fit is fit of the scan data. It defaults to 'best', but can also be assigned as 'cosine' or 'fourier'""" | ||
rotors = [ | ||
HinderedRotor(scanLog=Log('ethyl_scan_72.log'), pivots=[1,2], top=[1,3,4], symmetry=6, fit='best') | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth doing the same thing to examples/arkane/species/Toluene_Hindered_Rotor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to leave one example in with the explicit symmetry number (although it's well documented) for users who would like to specify it. But I'm open to other thoughts.
With test_rotor_symmetry_determination and a relocated test_gaussian_log_file_error Removed the TestStatmech class from commonTest.py
Done! |
Motivation or Problem
We'd like to allow Arkane to automatically determine rotor symmetry numbers. It is essential for automated thermo and rate codes using Arkane, like ARC. This feature is purposely added to Arkane and not ARC to benefit all users (if it works correctly, it could assist users to avoid making errors).
Description of Changes
A function for rotor symmetry determination was added to statmech.py.
The primary criterion used for determining whether a scan has symmetry or not is a comparison between peak/valley height difference and the worst peak/valley resolution.
This feature is able to differentiate between the following challenging cases, the one on the left is not symmetric (although it might look like it is at first sight), and the second has a symmetry number of 3:
Testing
Tests for the rotors plotted above for ethylamine were added. A
statmechTest.py
file was created.Reviewer Tips
Try running Arkane with two arbitrary rotor scans, one with symmetry and one without, and see (in the log file) whether the determined symmetry is correct.