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

Closes #171. Fix Park's correction to the relaxation time. #199

Conversation

mgoodson-cvd
Copy link
Collaborator

Fix Park's correction to the relaxation time.
The temperature term was inadvertently removed in bc9ce81.

Also updates the unit test for Millikan-White.

See #171. Closes #171.

@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #199 (f5a7f2c) into master (b7cfe9c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #199   +/-   ##
=======================================
  Coverage   71.22%   71.22%           
=======================================
  Files         135      135           
  Lines        8972     8974    +2     
=======================================
+ Hits         6390     6392    +2     
  Misses       2582     2582           
Impacted Files Coverage Δ
src/transfer/MillikanWhite.h 100.00% <ø> (ø)
src/transfer/MillikanWhite.cpp 90.47% <100.00%> (+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 b7cfe9c...f5a7f2c. Read the comment docs.

@jbscoggi
Copy link
Member

jbscoggi commented Feb 1, 2022

Hey @mgoodson-cvd, this is a great catch, thank you! I think it would make sense to actually move the calculation of sigma into the limitingCrossSection function which should take the temperature as the argument. As you have correctly noted, what it was returning is actually the reference value at 50,000K, not the real limiting cross section which is a function of temperature. For the temperature clip, I would make that an option as well for MillikanWhiteModelData class which will default to 20,000 K but have a setter function to change. Please also update the documentation for this class to point to the original Park paper. I think the reason I missed this is because this is not explicit in the Gnoffo paper, but it should be.

@mgoodson-cvd
Copy link
Collaborator Author

@jbscoggi That makes sense. Quick question on nomenclature and implementation -- right now you have a getter and setter for limitingCrossSection which by default is the omegav read from the database at data/VT.xml (typically 1e-20 or 3e-21 m^2). Is omegav the "limiting cross section", or is the "limiting cross section" the value with the temperature scaling? In the former case, we should probably add a new function to the MillikanWhiteModelData class, maybe effectiveCrossSection(double T) or correctedCrossSection(double T), that gets what we need for the transfer term. In the latter case, we'll want to rename whatever sets the non-scaled omegav, e.g. uncorrectedCrossSection.

@jbscoggi
Copy link
Member

Hey @mgoodson-cvd, sorry I didn't realize you replied the other day. I think the temperature scaled version is the actual limiting cross section, so let's call the setter something like setReferenceCrossSection(double omegav) and keep limitingCrossSection(double T) for getting the limiting cross section at a given temperature.

Fix Park's correction to the relaxation time.
The temperature term was inadvertently removed in bc9ce81.
Modifies the existing `limitingCrossSection` routine to take
the temperature as an argument to scale the reference cross
section.

See mutationpp#171. Closes mutationpp#171.
Update Millikan-White unit test to include the fixes
for the Park correction. Now checks both the reference
and limiting cross sections.

See mutationpp#171.
@mgoodson-cvd mgoodson-cvd force-pushed the 171-confusion-over-the-parks-correction-for-vibrational-relaxation branch from f16273a to f5a7f2c Compare February 11, 2022 14:41
@mgoodson-cvd
Copy link
Collaborator Author

@jbscoggi How's this look?

Copy link
Member

@jbscoggi jbscoggi left a comment

Choose a reason for hiding this comment

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

Perfect, thanks a lot.

@jbscoggi jbscoggi merged commit 222445c into mutationpp:master Feb 15, 2022
jbscoggi added a commit that referenced this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusion over the Park's correction for vibrational relaxtion
2 participants