-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed Reducing Water Issue in Transform Climatic Dialogue #8935
Fixed Reducing Water Issue in Transform Climatic Dialogue #8935
Conversation
@MeSophie good you are back to fixing this.
|
This reverts commit 6c3aa0c.
@rdstern Could you please test this dialogue again? |
@MeSophie it is getting closer, though I can't yet check the reducing part, see c) below: |
@rdstern The error message is due to missing values in the rain variable. |
@lilyclements and @MeSophie As you say, the problem of the -5 on day 1, when you allow for an evaporation variable should be examined, but there is a much larger problem when there are missing values in the rainfall variable. I am ok, if missing values in evaporation are not allowed, but we need to be able to "recover" from missing values in rainfall. Currently, if I have 1 missing day in 1935 for Dodoma, then thwe whole of the water variable is missing from then on. |
@rdstern I just want to confirm:
E.g.
This is currently what is done |
@rdstern I'm not getting the same issue as you are for dodoma. If one day is NA, I don't get that every day is NA.
In that code I have set day 10 (randomly) to be NA. |
@lilyclements that's a relief - I thought that was originally implemented. So let me check again on @MeSophie latest version. |
@lilyclements and @MeSophie I am using the latest version now. The R-code for the Dodoma data is here:
I added NA in January 1935 here: And it remains NA for ever. I don't see in the code above, how it could do otherwise? I then tried with a column, called evap, and got the same result. Then with reducing and it said missing isn't yet allowed. More generally I would like to have a new version of R-Instat as soon as this reducing evaporation issue, is ok, and evapotranspiration is confirmed to be working now. Then we can tell CIMH they could test. @jkmusyoka is evapotranspiration now ok? Perhaps that could be your starting point on the climatic guide, and the R-Instat help? |
@rdstern thanks for sharing. My mistake - I was looking at the "End of Seasons", not the "Transform" dialog. So, as you pointed out, we want to be running the bits where we replace the NAs with 0/max and check if the value is the same etc etc. @MeSophie the code that should run here is the same as the End of Seasons dialog - that is,
|
@lilyclements Okay thank you for this clarification. |
@lilyclements Please does this mean that I have to add the control water balance to the transform dialogue or should I just assign it the default value 0.50 in the code? |
@MeSophie from first glance it seems to be working on the missing values. Great: I hope you can fix it, because it is just that the reducing goes over 100. i.e. it isn't respecting the maximum value. I can't check how it is coping with missing, until you fix this. d) Finally, a small "tweak" that I'm sure you can do. The output looks "messy" with all those decimal places. Could you include round(water, 1) in the calculation. It could be added generally as the non-reducing will usually be to 0.1mm anyway. So the result is rounded each day. Great - I think it is almost there! |
@lilyclements Please could you have a look at c) of @rdstern comment ? Thank you. |
@lilyclements Please anything new about this? |
@lilyclements I wonder if @jkmusyoka should take this over, if you don't have time now? He is now in UK and I assume you will be meeting on e-PICSA soon? Meantime, @MeSophie your R is preet good. Have you tried to examine the problem yourself? |
@rdstern I am struggling to replicate your error - I have tried it with the same data, but I am getting it capped at the 100. Can you remember what parameters you used for this, and/or do you have a copy of the R code output? I am getting: ![]() Putting the R code here, in case it helps (probably future-me will appreciate this)
|
@lilyclements you can obtain the result buy using dodoma data Climatic Transform and reducing box.
|
@MeSophie thank you for sharing the code. From what I can see from inspecting your code against mine, the issue is in the wb_min (and similarly for wb_max) Yours currently runs: purrr::accumulate(.f= ~ pmin(pmax(..1 + ..2 - WB_evaporation(..1, 0.5, 100, 5, ..2), 0)), .x=tail(x=rain_min, n=-1), .init=0) But can you try the following here and in wb_max to see if it still has the issue?: purrr::accumulate(.f= ~ pmin(pmax(..1 + ..2 - WB_evaporation(..1, 0.5, 100, 5, ..2), 0), 100), .x=tail(x=rain_min, n=-1), .init=0) There’s a lot of brackets, I know. But essentially what you’re saying is to get the minimum out of “pmax(..1 + ………)” and nothing else. So it doesn’t have anything to cap it at. This change says to get the minimum out of “pmax(..1 + ………)” and “100”. This means that the water balance value caps at 100 if “pmax(……) > 100” |
@lilyclements Thank you the result looks good now. Please @lilyclements this 100 does this 100 come from the capacity control or it is invariable? Mean that I will also correct the End of Season dialog code? |
@MeSophie good question - yes, you're right it comes from the capacity control. |
@lilyclements Thank you. |
@rdstern @jkmusyoka Please could you test the dialog again ? Thank you |
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.
@MeSophie and @lilyclements brilliant - I think this is now working though I have just tested for Dodoma. So very well done both of you.
I am approving, though I still am afraid I have one last suggestion, but @N-thony could you check in parallel, so we can get this merged as soon as possible.
So I will discuss my suggestion with Lily because we will meet tomorrow. We need real payoff from all this work. I suggest it is our most ambitious use of the calculation system, and most people will not notice, because they will not be mirroring the commands. But, for our documentation and papers it would be really good if we added comments to the code! Sophie I presume you now understand enough to do this?
instat/dlgTransformClimatic.vb
Outdated
'Dim strEndofSeason As String = "end_of_season_combined" | ||
'Dim strDoyFilter As String = "doy_filter" | ||
'Dim strConditionsFilter As String = "conditions_filter" |
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.
remove?
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.
This is solved.
instat/dlgTransformClimatic.vb
Outdated
'clsRWaterBalanceFunction = New RFunction | ||
'clsRWaterBalanceFunction1 = New RFunction | ||
'clsRWaterBalanceFunction2 = New RFunction | ||
'clsWBEvaporation = New RFunction | ||
'clsTailFunction = New RFunction |
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.
remove?
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.
This is solved.
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.
@MeSophie I assume I am approving again. At the same time I record some actions I am taking to test the new dialog.
a) I introduced a missing value in the rainfall to check its action on the water balance:
This also confirms my request @MeSophie of the value for you to add comments to the R-code - perhaps in a new pull request.
b) I introduced a column - value 5 with the calculator, called evap - to check it is the same with a column as with a constant value.
c) Oh wow @MeSophie and @lilyclements I am no longer approving! But just request a very small change - which is now clearer to see as you have split up (I think) the R code, so it is clearer. I may be back-tracking on the need for more internal comments in the code - but I would like you to consider a small edit. Here is the result of the one missing value when you have reducing evaporation:
So, with reducing evaporation, it only seems to recover 200 days later - when there is a full profile, for the first time.
Now, in addition, here is your code from the script window:
That's beautiful, and it uses the R-Instat calculation system which we are now documenting! So this can become an excellent advert for R-Instat. And that's partly because I would like statistics packages to set an excellent example of coping well with missing values.
@MeSophie first, then maybe @lilyclements I am hoping that just a slight tweak in the code for wb_min. Currently I think you start with zero or max. But, with reducing evaporation you never get to zero, so the missing value trigger is only when the bucket is full?
So, let's start small instead - I want to keep thinks simple so let's start anyway (maybe reducing or not) with capacity/20 - so 5mm if capacity = 100?
Exciting, because this is timed with including documentation on the R-Instat functions in a linkable way from the help.
And we need to write this up! I'll discuss with @volloholic
@rdstern I have made a change to the code and it's now in the form:
|
Two questions
@MeSophie your R code change is good, but the R code now says that if we have a missing value in
Good point @MeSophie, I will wait to see @rdstern's comment on this. |
@lilyclements I noticed, when the water balance was recovering from a misssing value it seemed to be working really well in the non-reducing case. So what to do after missing values in the reducing case. The method now seemed not to find the values equal, when both (starting full and starting empty) at the low end. It only seems to work when both are full. I assume that's because the formula at the empty end is complicated, because of the reducing nature? I was wondering how to solve that? I'm not sure my starting at max/20 will help. Instead, could we try checking each day whether the abs(difference) (between starting full and starting empty) is less than a small amount - say 0.5mm rather than when they are equal? If we get something that works, we will then also need to use the code in the end of season dialog? Once this is finshed I would like us to write to CIMH to suggest we write this up jointly - not in the current contract, but nice as a paper. This could be you and Sophie here and Adrian and Lisa there? |
@rdstern I just want to check I understand we're discussing the right bit: Currently, when a rainfall value is missing:
You're suggesting instead that when a rainfall value is missing:
Am I understanding this correctly? Thanks! |
@lilyclements that sounds fine, though if it started at the lower limit previously, then maybe we keep it as before. I was under the impression it started at zero before as the lower point, and though this resulted in it never being equal, when both were at the lower limit. But it is also such a detail I'd mainly like to be able to close this. Though I am keen we contact Lisa and Adrian, to write up this stuff in a paper! |
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.
@N-thony I am now approving this one, as I suggest it is working sort of ok and we can continue checking once merged. @lilyclements I hope you agree?
So, @N-thony could you check anyway, and then merge unless Lily complains earlier.
Fixes #8891
@rdstern , @lilyclements I made some change on Transform> Water Balance dioalogue. Please have a look.
@lilyclements Could you please check the code of the windows script? I didn't introduce the .x and .int parameters because they weren't in the initial reduce function. I don't know if this is the script to get because the summary of the result is very large compared to the End of Saison Dialogue.