-
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
Fix kinetics fitting with Tmin & Tmax in Arakne #1672
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1672 +/- ##
==========================================
- Coverage 41.88% 41.86% -0.02%
==========================================
Files 176 176
Lines 29368 29368
Branches 6059 6059
==========================================
- Hits 12301 12296 -5
- Misses 16188 16192 +4
- Partials 879 880 +1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1672 +/- ##
==========================================
+ Coverage 41.87% 41.88% +0.01%
==========================================
Files 176 176
Lines 29369 29369
Branches 6059 6059
==========================================
+ Hits 12297 12302 +5
+ Misses 16192 16188 -4
+ Partials 880 879 -1
Continue to review full report at Codecov.
|
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.
Thanks! Please see some comments
arkane/kinetics.py
Outdated
else: | ||
self.Tmin = None | ||
self.Tmin = (298, 'K') |
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.
Could you simplify these assignments into:
self.Tmin = Tmin if Tmin is not None else (298, 'K')
?
Same for Tmax?
We could also simplify Tcount:
self.Tcount = Tcount if Tcount > 3. else 50
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.
Good idea. I just added that.
a5f77f5
to
0fa0afb
Compare
Previously, there were two spots where a list of temperatures was created for fitting high pressure kinetics in Arkane. One fit using inverse temperature spacing and the other one fit with linear temperature spacing. See #1671 This commit moves the location for creating a listing of temperatures to the __init__ method, so there is only one spot where temperatures are initialized. The default min and max in `generateKinetics` was moved into the __init__ method. Three unittests were written for obtaining the proper temperatures in the __init__ method. `generateKinetics` previously accepted an arguement Tlist, which is already an attribute of KineticsJob, creating redundancy. The Tlist argument was removed and self.Tlist was used in this method instead, simplifying the code. Calls to quantity.Quantity in the __init__ method were unnecessary before setting them with quantity.Temperature, so the __init__ code was simplified by setting attributes with (value, 'K') instead. Documentation was updated to better describe the three methods of specifying temperature when calling `kinetics` in Arkane
0fa0afb
to
46ae3d7
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.
Thanks!
Motivation or Problem
Previously, there were two spots where a list of temperatures
was created for fitting high pressure kinetics in Arkane.
One fit using inverse temperature spacing and the other one fit
with linear temperature spacing. See #1671
Description of Changes
This PR consolidates creating a listing of temperatures
to the init method, so there is only one spot where temperatures
are initialized. The default min and max in
generateKinetics
wasmoved into the init method. Three unittests were written for
obtaining the proper temperatures in the init method.
generateKinetics
previously accepted an arguement Tlist, whichis already an attribute of KineticsJob, creating redundancy.
The Tlist argument was removed and self.Tlist was used
in this method instead, simplifying the code.
Calls to quantity.Quantity in the init method were unnecessary
before setting them with quantity.Temperature, so the init
code was simplified by setting attributes with (value, 'K') instead.
Documentation was updated to better describe the three methods
of specifying temperatures when calling
kinetics
in ArkaneTesting
did
make test
and ran the kinetics examples in RMG. also didmake html
to check for errors in the documentation.Reviewer Tips
Read over code to make sure it makes sense.