-
Notifications
You must be signed in to change notification settings - Fork 51
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
Proper use of loc
parameter in gamma
,fisk
dists (SPI/SPEI)
#1720
Conversation
I started a new branch |
Apparently the PR was never in draft mode? Just wanted to say it's ready for your eyes. |
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
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.
I like this! thanks a lot !
if method == "APP": | ||
fitkwargs["floc"] = 0 | ||
# same offset as in climate indices | ||
offset = convert_units_to("1 mm/d", wb, context="hydro") |
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.
offset = convert_units_to("1 mm/d", wb, context="hydro") | |
offset = convert_units_to("1000 mm/month", wb, context="hydro") |
Isn't this the hardcoded offset of climate_indices
?
https://github.com/monocongo/climate_indices/blob/d108eee982abae06a415e888319b8078af868558/src/climate_indices/indices.py#L280C62-L280C69
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.
yes, climate_indices and xclim do match, but with the current datasets used in our test, we stil have negative values with the climate_indices offset. I wll wait for a next PR where we show many comparisons to many other libraries to sort this out
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.
but I agree, I didn't have the correct offset here
Co-authored-by: Pascal Bourgault <bourgault.pascal@ouranos.ca>
<!-- Please ensure the PR fulfills the following requirements! --> <!-- If this is your first PR, make sure to add your details to the AUTHORS.rst! --> ### Pull Request Checklist: - [x] This PR addresses an already opened issue (for bug fixes / features) - This PR fixes #xyz - [x] (If applicable) Documentation has been added / updated (for bug fixes / features). - [x] (If applicable) Tests have been added. - [x] CHANGES.rst has been updated (with summary of main changes). - [x] Link to issue (:issue:`number`) and pull request (:pull:`number`) has been added. ### What kind of change does this PR introduce? * Tests for the GIS module were too strict. * Changes were made in `xclim v0.49.0` with how the `gamma` and `fisk` distributions were initiated in `xclim`, which changed the fitting results and parametric quantiles. It just so happened that we arbitrarily used the `gamma` for our tests. To be backwards-compatible, those tests were changed to the `gumbel_r`. ### Does this PR introduce a breaking change? - No. ### Other information: Changes to the `gamma` distribution: Ouranosinc/xclim#1477 Ouranosinc/xclim#1720
Pull Request Checklist:
number
) and pull request (:pull:number
) has been addedWhat kind of change does this PR introduce?
gamma, fisk
. Now, instead, we properly use theloc
parameter in those distributions that generalize the distributions and allow negative values. For instance, thegamma
disitribution is a 2-parameter function, but inscipy
it's generalized to 3 parameters, with aloc
parameter. This extends the support ofgamma
fromThe code was already compatible with this idea, just had to change
_fit_start
so it can compute estimates of the parameters in the cases with negatives values.Does this PR introduce a breaking change?
Removing the offset from SPEI. Also, the
_fit_start
can now compute an estimate ofloc
which can affect what value we obtain when optimizing 3-parameters distributions. Even when theloc
is fixed to 0 withfloc=0
infitkwargs
, the estimate offisk
was modified. The previous formulation was obtained by applying more or less the idea in the method of moments, but now I obtained an approximation that's more rigorous.The case of zero inflated distribution has been changed: Now, it is the calibration data that determines the probability of zero, as it should.
Method
APP
can now only be used iffitkwargs['floc']
is given as input by the user.Other information: