Remic/raincess #351
Remic/raincess #351
Conversation
There was a problem hiding this comment.
Looks good so far (see PR description for more work that needs to be done).
I'm a little annoyed that this repeats some of the logic from the water_balance function rather than reusing it, but I'm not sure we can fix that without making the API more complicated to use. I might work on it in a separate PR (or not).
|
How do you run the tests? When I run |
|
Got it, |
|
As you mentioned in the PR description, this is incomplete--the maprooms have to be adapted to the API change. I have some comments that need to be addressed before that happens, and some that can be addressed after or in parallel. In #371 I have suggested a different API. We should reach agreement on that before changing the maprooms (although it wouldn't be the end of the world if we had to change the maprooms twice--I don't think the change will be very big). Comments that can be addressed separately because they don't affect the API:
|
|
Oh, the other thing is that seasonal_cess_date is currently hard-coded to use pre-calculated soil moisture. Presumably you plan to give it the option to calculate from rain? Maybe that's part of adapting the maprooms to the API change. |
|
ah... yes... It seems I only thought through part of it... |
|
Pushing so I can pull on shortfin01 and adapt Maprooms |
|
it still took some time for cess date dependent images to show up (cess date and lengths ts and length map), on shortfin01... It's not going to be the exact same results because sminit now is initialized every year, whereas before we had a full time stream of sm so we had a computed sm for the date the search starts. Something to discuss with hydrologists when we will actually have users for these. For now, I think it's ok. |
Hopefully the results aren't very sensitive to starting with sminit vs. starting with the computed value from the previous year. If they are sensitive to it then it seems like a problem, because errors will accumulate. |
From my experience with past simulations, it's not very sensitive with intialization, except for the 1st few days so it would impact finding very early cessation dates from the search date. If the search date is like 1 month from the end of the rainy season, it would not matter, if someone plays with starting their search later, then at what value they initialize may matter. I think it's a matter of communicating that in the App, and maybe give the initialization as input choice. Note that the difference is not with starting with value computed from previous year. The way it was set up before, I was computing the entire 40 years of daily soil moisture. Then if you start searching on April 1st, it was starting with the April 1st value of that year (or March 31st of that year). |
|
Just adding a reminder that we might want to apply the new logic for cess_date to water_balance |
|
Shall we merge this? Porting to water balance can come later, as well as a applying multiple-year seasonal operators when they are ready. |
|
@aaron-kaplan , I don't think the changes here are needed to progress on the water balance work, but since I also moved files around, it may be better to merge sooner than later or it will be painful to rebase... |
|
I scanned through the comments and I didn't find anything that hasn't been addressed. Do you know what this was waiting for? Github says there are conflicts. Please rebase so I can review what will actually happen when we merge. |
remicousin
force-pushed
the
remic/raincess
branch
from
30b6732 to
2044710
Compare
|
rebased, removed a print from the tests. Passed all the tests (arccos warning is there but being fixed in other PR). Maprooms (tested Senegal "onset" and "wat_bal") seem to work fine. Bug to click ts to update map still there but being fixed in other PR. Cessation dates and length of season are not timing out on shortfin01 and calculated. I think there was nothing else to wait for but your last scan and then your clicking the approve button. |
|
Whoops, after approving I noticed that there are still conflicts. |
remicousin
force-pushed
the
remic/raincess
branch
from
2044710 to
3ba9fe6
Compare
|
rebased |
|
@aaron-kaplan we probably want to merge this to facilitate the likely new or follow-up work coming up |
|
I am going to rebase |
remicousin
force-pushed
the
remic/raincess
branch
from
3ba9fe6 to
d2ecc45
Compare
|
Rebased... there was no conflict to fix manually |
|
Did you test after rebasing? This was forked a long time ago, so I wouldn't be surprised if something incompatible happened in the meantime. |
|
I built successfully the Water Balance and Growing season Maprooms for Senegal, after having changed in a couple of places inputs of functions such as et=5 back to just 5. I think we wanted that to ease readability but it failed and complained that this type of argument must be last. That bug it also on master though. So could change it here, or first thing first after merging this. |
|
Are you saying those maprooms are broken on master? How did we break them without noticing? If you have the changes all ready, might as well add them to this branch. |
|
Water Balance built is triggered only in Senegal config and what's currently there has probably not been updated in ages longer than this PR. |
|
But I can and will make the change here. |
|
done |
|
Do you want to deploy this to ANACIM (or elsewhere)? If so, I'll build and publish a new docker image. Otherwise I'll wait until we need it, by which time there may have been other changes too. |
|
We need not deploy anything for now. There is more to come and Jim said we can deliver end of June. At that time, there might be improvements that make the cessation date calculations (and those derived from) more efficient or usable at all on all partners' we wanted to install to. And there will be some Water Balance products. |
This adds the option to have cess_date driven by rain rather than sm.
It made me re-arrange the test and agronomy files to be more coherent with the new set up for multi-page apps.
It turns out I needed not the water_balance with reduction but rather used directly water_balance_step.
The test is creating daily_rain data that would yield same sm (with et=5 and sminit=0) than the test with sm directly.
To be coherent with previous work, I will introduce stuff like et=xr.DataArray(et) so that the user can input numbers. I've just realized I've forgotten doing that.
This will break the maprooms since there is an additional (boolean) argument in cess_date, but I will correct the maprooms in this PR once we are good with the function. And since I'll have to do that, I'll rewrite the maprooms to use the rain as input as opposed to computing the full array of sm first. (at least for the case where it doesn't do multiple years, the multiple years case is covered by the changes brought up to the seasonal functions in this PR). Then in another PR, we can either work on the multiple years here, or pause on that and apply all those stepping changes to the "real" water balance function and use that one for good. Either way.
R