Fix Perfect Model Ice Nucleation Calibrations #457
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
=======================================
Coverage 96.95% 96.95%
=======================================
Files 41 41
Lines 1543 1543
=======================================
Hits 1496 1496
Misses 47 47
|
| # test that coeffs are close to "true" values | ||
| if IN_mode == "ABDINM" | ||
| TT.@test calibrated_parameters[1] ≈ coeff_true[1] rtol = FT(0.3) | ||
| TT.@test calibrated_parameters[2] ≈ coeff_true[2] atol = FT(3) |
There was a problem hiding this comment.
Is there a reason why one value is checked with relative and the other with absolute tolerance?
There was a problem hiding this comment.
The c coefficient can be pretty small so using relative tolerance up to 0.70 won't pass the test even though the value is not horribly calibrated (i.e. true c coefficient for ABDINM is ~2 but calibrated c will be ~4 which doesn't seem too bad to me), so I switched to atol. The m coefficient is ~20 to ~255 so it's easier to use rtol and have the tests pass.
To force the tests to pass using rtol I could:
- Make the noise of the pseudo data really small and there will be a higher probability that the test for the c coefficient will pass for say rtol = 0.3, but there's still a chance that the calibration will decide to have a relatively large or small guess for a specific ensemble (plus at that point, I'm just making the pseudo-data noiseless which seems to defeat the purpose of the perfect model tests in the first place).
- Make the standard deviation for the priors very small which would also defeat the purpose of calibration since I'm basically giving it the true value to start with.
At some point I was tired of restarting the CI checks until the tests pass so I changed the c coefficient test to atol instead of rtol. If you have a cooler fix please let me know 🙃
parcel/ParcelModel.jl
Outdated
| @@ -17,6 +17,7 @@ Base.@kwdef struct parcel_params{FT} <: CMP.ParametersType{FT} | |||
| deposition_growth = "None" | |||
| liq_size_distribution = "Monodisperse" | |||
| ice_size_distribution = "Monodisperse" | |||
| σ_g = FT(0) | |||
There was a problem hiding this comment.
When droplets or crystals are set to lognormal distribution. Lognormal distribution wasn't tested anywhere so I didn't catch that it was missing earlier. Not specifically related to this PR
| @@ -167,7 +264,7 @@ function calibrate_J_parameters(FT, IN_mode, params, IC, y_truth, Γ,; perfect_m | |||
|
|
|||
| prior = create_prior(FT, IN_mode, perfect_model = perfect_model) | |||
| N_ensemble = 10 # runs N_ensemble trials per iteration | |||
| N_iterations = 15 # number of iterations the inverse problem goes through | |||
| N_iterations = 150 # number of iterations the inverse problem goes through | |||
There was a problem hiding this comment.
Think I needed it before when the calibrations weren't working right and things didn't converge early enough. I can change it to 75 but if I set it to anything lower, one of the experiments doesn't seem to really converge (IN05_19)
|
Left some small comments, feel free to squash and merge afterwards. I'm probably not best equipped to review calibrations setup. It might be good to ask @odunbar when he is back to take a look. But parcel model wise everything looks good! |
Thanks, Anna! I should've rebased this branch onto the other AIDA hom branch but there's a lot of changes here that should be attached to PR #385 instead of this one. I've tagged Ollie in that one just now, but I think I still need your approval before GitHub lets me merge that one |
amylu00
force-pushed
the
al/fix_perf_IN
branch
5 times, most recently
from
d813a6f
to
ed40e36
Compare
amylu00
force-pushed
the
al/fix_perf_IN
branch
from
3aa4a15
to
e8f20d9
Compare
amylu00
force-pushed
the
al/fix_perf_IN
branch
from
9f21897
to
fc044ab
Compare
to be merged only after AIDA HOM calibrations are merged!