-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
add isotopic ventilation ratio to physics (f_heavy to f_light) #1532
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1532 +/- ##
==========================================
- Coverage 85.41% 85.40% -0.01%
==========================================
Files 389 391 +2
Lines 9514 9526 +12
==========================================
+ Hits 8126 8136 +10
- Misses 1388 1390 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
): | ||
return ( | ||
const.FROESSLING_1938_A | ||
+ const.FROESSLING_1938_B * sqrt_re_times_cbrt_sc * diffusivity_ratio |
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 intentional to hardcode the Froessling formula here?
would it be possible to make it usable with other ventilation coefficient formulations? (like the Pruppacher & Rasmussen?)
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 should definitely be written to use both ventilation formulae.
For now it can be done by adding diffusivity ratio to Froessling and preset it to 1? With a note where it can be used?
The other way is to reuse ventilation formula but then we need to use Froessling constant again.
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.
isn't the diffusivity coefficient implicitly part of the Schmidt number?
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.
Oh yes! Definitely it is!
@@ -86,6 +87,7 @@ def __init__( # pylint: disable=too-many-locals | |||
self.isotope_diffusivity_ratios = isotope_diffusivity_ratios | |||
self.isotope_relaxation_timescale = isotope_relaxation_timescale | |||
self.isotope_temperature_inference = isotope_temperature_inference | |||
self.isotope_ventilation_ratio = isotope_ventilation_ratio |
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.
would isotope_ventilation_coefficient_ratio
be a better name?
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.
Maybe only isotope_ventilation_coefficient
? Depends on what we decide about isotope_ventilation_coefficient
function. Right now it is inside this folder with ratio
function.
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.
given the congruence of the Sherwood number and the "n" turbulence factor theory, let's perhaps aim for a folder name that does not mention "ventilation", but rather covers both the "f" and the "n" treatments and nomenclature? So that a user can switch between the "n" and the "f" treatments? Would it be possible?
No description provided.