Closed dburov190 closed 3 years ago
NB: I have to admit, I haven't tested this yet; if anyone wants to, please go ahead. Otherwise, I'll test it in a couple hours.
If it runs then it is fine with me to merge. As these are all small fixes, do any of them actually require python 3.8.5 to run? If so then we should change the package requirements
No-no, the change to 3.8.5 was only to test it on cluster. On the other hand, 3.7.0 (which was used previously) is also not on the requirements list. Anyway, the code itself runs fine on 3.x, probably 3.6+.
UPD works fine, safe to merge. I'm writing another fix right now though, to fix inconsistencies with DA times as well.
Done working on the second fix; please review (:
@odunbar Regarding renaming: you can adopt the changes that I made to true_rates_estimation.py
here or edit it the way you need it for ROC plots, and keep only one file. I would prefer the latter since we don't really need a separate file that does almost the same, but just has an additional plotting section. I mean, once this is merged, you can just edit true_rates_estimation.py
the way you like it.
you can adopt the changes that I made to true_rates_estimation.py here or edit it the way you need it for ROC plots, and keep only one file. I would prefer the latter since we don't really need a separate file that does almost the same, but just has an additional plotting section. I mean, once this is merged, you can just edit true_rates_estimation.py the way you like it.
RE consistency of the DA: we do the same now in terms of
T_0
, T_1
we save discrete steps in [T_0,T_1)
We are inconsistent in one main regard:
I think it is most useful for us to merge this and then I'll combine changes and remove this estimation file in my PR, Then I can merge that and we can begin the 'scientific' testing again in a parallel fashion on that setup.
@odunbar Alright, I just moved the prediction phase up in the main loop, so that it comes before the DA sweeps — this removes inconsistency with your script. I've tested it, doesn't crash, the plots are reasonable... merging then.
This short PR is to fix the storing inconsistency, i.e.:
[current_day + 0.125, current_day + 0.25, ..., current_day + 1.0]
which means that if we compare at round days, we compare predictions with kinetic values[current_day, current_day + 0.125, ..., current_day + 0.875]
(including ICs), and then, at the very last step, an additional ensemble state attime_end
(for example, 30.0).So, the amount of stored values is now +1, because we didn't store ICs previously (which was a minor bug anyway).
Also, this PR will also rename
accuracy_estimation
->true_rates_estimation
and change the code accordingly (because accuracy is useless anyway).Finally, add a couple timer outputs (for spin-up and main-loop windows).
UPDATE: This now also includes a fix for incorrect assimilation times:
current_day - 0.125
); thus,H
andD
would always be assimilated with a one-day delay on the backward-assimilation run (the forward assimilation was fine though);H
andD
, and depending onI_assimilation_delay
, ofI
as well), then the ensemble is integrated backwards until next day, assimilated, and so on.This results in 1 more assimilation per DA window, because we will now have
2*K + 1
assimilation updates, whereK
is the number of days in a DA window:K
times backward, then once at the time furthest from now, thenK
times forward (remember the fence post problem?).I've also accidentally made the DA window code ~27% faster by putting ensemble updates under if-statements (only update when assimilation was carried out).