Derive the frequency scaling factor in Arkane from the frequency level #1612

alongd · 2019-06-02T00:57:05Z

Motivation or Problem

If the user did not specify the frequency scaling factor, Arkane uses the model chemistry to determine it from a dictionary. However, the model chemistry in Arkane currently respesents the sp level, whereas the freq level should have been used instead.

Description of Changes

Arkane can now process a model chemistry in an sp//freq form, e.g., CCSD(T)-F12a/aug-cc-pVTZ//B3LYP/6-311++G(3df,3pd), and determines the frequency scaling factor from the freq level, if available.
Also, if the frequency scaling factor was not specified and could not be determined from the dictionary, we now raise an error instead of setting it to 1 and warn the user. This way, software like ARC could capture the error and attempt to derive the frequency scaling factor by itself.

Testing

Added an example which also runs in RMG's tests. Also added a unit test for the new process_model_chemistry function.

codecov · 2019-06-03T01:00:11Z

Codecov Report

Merging #1612 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1612   +/-   ##
======================================
  Coverage    41.6%   41.6%           
======================================
  Files         176     176           
  Lines       29142   29142           
  Branches     5990    5990           
======================================
  Hits        12125   12125           
  Misses      16173   16173           
  Partials      844     844

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 f90526f...b7967ba. Read the comment docs.

alongd · 2019-06-03T01:06:20Z

I changed the default behaviour if the freq scale factor isn't found, back to what it was before this PR: of setting it to 1 and warning the user instead of crushing. I think it makes more sense because there are cases where the user doesn't care about the freq scale factor, such as running an RMG network file, or running the Explorer tool. We shouldn't make Arkane crush if the user doesn't artificially set the factor to 1 in the input file.

Also, thanks @dranasinghe for spotting this issue to begin with!

I think it should be ready for review now.

alongd · 2019-06-03T14:17:25Z

Added more unit tests to Arkane's inputTest (force pushed)

cgrambow

Looking good. I made a comment regarding what I think the default behavior should be. Also, you mean "crash", not "crush" 😄

cgrambow · 2019-06-03T17:35:35Z

arkane/input.py

+    else:
+        # assume only the sp level was given in the model chemistry, assign to sp and return None for freq
+        sp_level = model_chemistry
+        freq_level = None


I don't think this is the desired default behavior. If only one level of theory is specified, it should be assumed that frequency and energy calculations were done at that level of theory. This would also match typical descriptions in literature. For example, if the user specifies wb97x-d3/def2-tzvp, then sp_level and freq_level should both be set to that.

You can then combine the '/' not in model_chemistry case and the else case into just one else case.

I'll change accordingly, but we still need the / that differentiates this from a composite method

In both cases we set sp_level and freq_level to the same thing, so we don't have to check composite methods separately.

I agree, added a fixup

cgrambow · 2019-06-03T17:44:27Z

arkane/statmech.py

-        logging.warning('No frequency scale factor found for model chemistry {0}; assuming a value of unity.'.format(
-            model_chemistry))
-    else:
+    if model_chemistry in freq_dict:


As far as I can tell, your code changes here don't actually change any of the functionality. Why are your changes necessary? Can we just leave the code as it was?

cgrambow · 2019-06-03T17:54:19Z

arkane/inputTest.py

+
+        spc0 = species(label0, **kwargs)
+        self.assertEqual(spc0.label, 'CH2O')
+        self.assertEqual(spc0.molecule[0].toSMILES(), 'C=O')


You can use @mliu49's new SMILES attribute now and just do spc0.SMILES.

cgrambow · 2019-06-03T17:58:19Z

arkane/inputTest.py

+        mc = 'ccsd(t)-f12a/aug-cc-pvtz'
+        sp, freq = process_model_chemistry(mc)
+        self.assertEqual(sp, 'ccsd(t)-f12a/aug-cc-pvtz')
+        self.assertIsNone(freq)


Change this dependent on your changes to my other comments.

alongd · 2019-06-03T18:47:39Z

Thanks for the comments! I added fixups, let me know when to squash

cgrambow

Looks good! I just have a tiny comment about some wording in a comment. Other than that, you can squash and I think you'll also need to rebase.

cgrambow · 2019-06-03T19:46:48Z

arkane/input.py

@@ -706,7 +706,7 @@ def process_model_chemistry(model_chemistry):
        # assume this is an sp//freq format, split
        sp_level, freq_level = model_chemistry.split('//')
    else:
-        # assume only the sp level was given in the model chemistry, assign to sp and return None for freq
+        # assume the sp freq levels are the same, assign to the model chemistry to both


I think some words might have gotten lost here. You probably meant something like assume the sp and freq levels are the same, assign the model chemistry to both

And modified existing examples

alongd · 2019-06-03T19:55:16Z

Thanks!! Rebased and squashed

mliu49 · 2019-06-03T21:35:03Z

arkane/input.py

+        sp_level = freq_level = model_chemistry
+        if model_chemistry.startswith('cbs-qb3'):
+            # hard code for CBS-QB3-Paraskevas which has the same frequency scaling factor as CBS-QB3
+            freq_level = 'cbs-qb3'


Sorry about seeing this after merging, but this hard-coding is slightly redundant since there's an entry for cbs-qb3-paraskevas in assign_frequency_scale_factor (https://github.com/ReactionMechanismGenerator/RMG-Py/blob/master/arkane/statmech.py#L1224).

I think we could remove one or the other.

Sorry, didn't check the assign_frequency_scale_factor function in detail.

You're absolutely right, I haven't noticed the redundancy either.

alongd requested a review from cgrambow June 2, 2019 00:57

alongd self-assigned this Jun 2, 2019

alongd added the Arkane label Jun 2, 2019

alongd force-pushed the model_chemistry branch 2 times, most recently from 53686fa to bf43243 Compare June 3, 2019 00:17

alongd force-pushed the model_chemistry branch from bf43243 to 5d1f8a6 Compare June 3, 2019 14:16

cgrambow requested changes Jun 3, 2019

View reviewed changes

alongd force-pushed the model_chemistry branch from 366dcbd to 48d7ef1 Compare June 3, 2019 19:53

alongd added 4 commits June 3, 2019 15:53

Use the freq (not sp) level when determining the freq scale factor

f8491eb

Tests: Added input test to Arkane

7079b99

Modified Arkane documentation, explaining the model chamistry

8f773d7

Added an Arkane species example with sp//freq format model chemistry

b7967ba

And modified existing examples

alongd force-pushed the model_chemistry branch from 48d7ef1 to b7967ba Compare June 3, 2019 19:54

cgrambow approved these changes Jun 3, 2019

View reviewed changes

cgrambow merged commit 67dab34 into master Jun 3, 2019

cgrambow deleted the model_chemistry branch June 3, 2019 21:18

mliu49 mentioned this pull request Jun 3, 2019

RMG v2.4.0 (?) Release Planning #1591

Closed

5 tasks

mliu49 reviewed Jun 3, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive the frequency scaling factor in Arkane from the frequency level #1612

Derive the frequency scaling factor in Arkane from the frequency level #1612

alongd commented Jun 2, 2019

codecov bot commented Jun 3, 2019 •

edited

Loading

alongd commented Jun 3, 2019 •

edited

Loading

alongd commented Jun 3, 2019

cgrambow left a comment

cgrambow Jun 3, 2019

alongd Jun 3, 2019

cgrambow Jun 3, 2019

alongd Jun 3, 2019

cgrambow Jun 3, 2019

cgrambow Jun 3, 2019

cgrambow Jun 3, 2019

alongd commented Jun 3, 2019

cgrambow left a comment

cgrambow Jun 3, 2019

alongd commented Jun 3, 2019

mliu49 Jun 3, 2019

cgrambow Jun 3, 2019

alongd Jun 4, 2019

Derive the frequency scaling factor in Arkane from the frequency level #1612

Derive the frequency scaling factor in Arkane from the frequency level #1612

Conversation

alongd commented Jun 2, 2019

Motivation or Problem

Description of Changes

Testing

codecov bot commented Jun 3, 2019 • edited Loading

Codecov Report

alongd commented Jun 3, 2019 • edited Loading

alongd commented Jun 3, 2019

cgrambow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented Jun 3, 2019

cgrambow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alongd commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2019 •

edited

Loading

alongd commented Jun 3, 2019 •

edited

Loading