Correct equality used in ECC transformation approach - #2416
Conversation
…alues using the gamma distribution. Otherwise, negative stochastic noise values are included, which isn't the intention.
mo-jbeaver
left a comment
mo-jbeaver
left a comment
There was a problem hiding this comment.
All tests passed successfully, just added a few comments to the unit test for clarification.
| err_msg="Negative intensity value should map to approximately zero in gamma CDF", | ||
| ) | ||
| # The right column has positive value (2.0) and should map to a percentile | ||
| # close to 0.037 for this test setup. |
There was a problem hiding this comment.
Where has the 0.037 come from?
There was a problem hiding this comment.
I've extended this comment, so that it's clearer where the 0.037 has come from.
|
|
||
| # The key assertion: negative values in input produce very low percentiles (near 0) | ||
| # because gamma CDF on negative values returns 0. This confirms negative values | ||
| # are masked in mean/std calculation but still evaluated by gamma CDF. |
There was a problem hiding this comment.
Should there also be a check for the mean/std calculation?
There was a problem hiding this comment.
I don't think that I need to explicitly calculate the mean and standard deviation in this unit test but I've rewritten this comment to, hopefully, be a bit clearer about how the numbers in the result are calculated.
brhooper
left a comment
brhooper
left a comment
There was a problem hiding this comment.
Thanks @gavinevans, I've added one minor comment.
| result[:, 0], | ||
| [0.0, 0.157, 0.843], | ||
| atol=1e-3, | ||
| err_msg="Negative intensity value should map to zero in gamma CDF", |
There was a problem hiding this comment.
This error message should be updated to reflect what is being tested.
There was a problem hiding this comment.
I've made a minor update to this error message.
mo-jbeaver
left a comment
mo-jbeaver
left a comment
There was a problem hiding this comment.
This is a lot easier to follow now, thanks!
brhooper
left a comment
brhooper
left a comment
There was a problem hiding this comment.
Thanks @gavinevans
One more small comment.
| ), | ||
| ) | ||
| # Check that all values are finite | ||
| assert np.all(np.isfinite(result)) |
There was a problem hiding this comment.
I think we're explicitly checking the value of every point in result now, so can this line be removed?
There was a problem hiding this comment.
I've removed this assertion.
3 participants
Related issues: #2303
Description
This PR modifies an equality that is used as part of the gamma distribution fitting, which is part of the ECC transformation approach, where a gamma distribution is fitted for each grid point. The issue was that stochastic noise (as a negative value) had been added to the realization forecast prior to this step, however, this negative stochastic noise wasn't intended to affect the gamma distribution fitting, which has only intended for "real" precipitation forecast values. The presence of negative values in the inputs was skewing the mean and standard deviation estimate for the gamma distribution and giving output that didn't look quite as intended i.e. higher precipitation values than expected. To address this the equality used when considering the
nan_mask_valueargument has been made "<=", so that values equal to and less than this value will be set to NaN for the gamma distribution fitting section.Testing: