Add immersion freezing #205
Conversation
remove old directories
remove old directories
remove old directories
remove old directories
remove old directories
remove old directories
jcurtis2
left a comment
jcurtis2
left a comment
There was a problem hiding this comment.
Just a few more things I've noted while taking a pass through for PyPartMC.
src/env_state.F90
Outdated
|
|
||
| !> compute saturated vapor pressure (units : Pa) with respective to water | ||
| !> Formula (10) from [Murphy & Koop, 2004] (https://doi.org/10.1256/qj.04.94) | ||
| real(kind=dp) function env_state_saturated_vapor_pressure_water(T) |
There was a problem hiding this comment.
I also wonder why env_state isn't passed to this function given it is an env_state function?
There was a problem hiding this comment.
Because this function only requires the temperature, T is the only necessary input. I thought you had previously suggested that I use T instead of env_state as the input—should I change it back to env_state?
There was a problem hiding this comment.
Yes, you are right - I see where I suggested that. This was probably the simplest suggestion at the time as you had two functions that did the same thing (which isn't a great idea), one taking temperature and the other taking env_state.
I think that made this pretty confusing and in hindsight, we probably should apply a different fix. Not 100% sure what it should be at the moment.
The fact is, that all env_state functions take env_state as an input so this function is an outlier. So I think we need it to take env_state. However we also shouldn't have two functions that do exactly the same thing.
Two paths that I can think of:
- Make an
env_statein the theoretical_freezing.F90 code and then call those functions by passing anenv_statethat has it the temperature set. Then switch to have env_state as the input rather than temperature. - A function that takes env_state that is just a wrapper to call another function that just takes temperature (and this function name shouldn't start with env_state). However, I don't know that we have any cases in the code that does something quite like that. And I'm not quite sure what file this function would actually exist in (it doesn't quite feel like it belongs in util.F90).
There was a problem hiding this comment.
Thanks for your reply! I prefer the second approach since it wouldn’t complicate theoretical_freezing.F90. However, it would still be somewhat of an outlier. We can discuss this further.
src/run_part.F90
Outdated
| if (run_part_opt%do_immersion_freezing .and. & | ||
| (run_part_opt%immersion_freezing_scheme_type .eq. & | ||
| IMMERSION_FREEZING_SCHEME_SINGULAR)) then | ||
| call ice_nucleation_singular_initialize(aero_state, aero_data) |
There was a problem hiding this comment.
this is now not yet called if using timestep or timeblock variants -> should be moved to the timestep logic
Co-authored-by: Jeffrey Curtis <jcurtis2@illinois.edu>
Co-authored-by: Jeffrey Curtis <jcurtis2@illinois.edu>
…tate_saturated_vapor_pressure_wrt_water
|
closing and reopening to trigger codecov analysis in further commits |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #205 +/- ##
===========================================
+ Coverage 0 77.40% +77.40%
===========================================
Files 0 55 +55
Lines 0 9427 +9427
===========================================
+ Hits 0 7297 +7297
- Misses 0 2130 +2130 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| add_test(test_tchem ${CMAKE_BINARY_DIR}/test_run/run_test_directory.sh tchem) | ||
| endif() | ||
| add_test(test_weighting ${CMAKE_BINARY_DIR}/test_run/run_test_directory.sh weighting) | ||
| #>>>>>>> 2341ef410d6f49f3169b8461b5fa8c89dbd3c7a2 |
There was a problem hiding this comment.
this seems to be a merge-conflict leftover to be removed?
There was a problem hiding this comment.
would it be better to make it a part of the web-publishable doxygen html?
There was a problem hiding this comment.
IIUC, the lack of these additions was not detected by CI, it would be great to add a mechanism at least parsing all the scenario input on CI
There was a problem hiding this comment.
Yeah, this is a fairly common occurrence. The urban_plume scenarios are often fixed but the scenarios that often require more libraries (SUNDIALs for condense, CAMP) are often overlooked. This is often as simple as missing a spec file line change/addition. In this case, it would be a lack of adding things in aero_data.dat for most scenarios or a more special case in terms of the CAMP scenario. The CAMP test caught this, it was fixed in the test but not in the scenario til later.
I think that something that simply attempts to run a 0 second simulation will work in terms of reading in all inputs.
4 participants
Add the immersion freezing simulation codes, freezing test case, and freezing scenario.