Change lmoments3 behaviour with fitkwargs #2045
Conversation
|
@aulemahal Sascha suggested an error instead of a warning, thoughts? Sometimes I'm not sure which is more appropriate |
There was a problem hiding this comment.
I was thinking of a warning because I was thinking of a usage where one calls the function multiple time with different distributions but the same other arguments. I realize now that this does not make much sense, woups.
Thinking of it, an error might indeed be better. Sorry for the confusion!
Co-authored-by: Pascal Bourgault <[email protected]>
|
Now, we can use PWM in SPI/SPEI functions directly. I added tests: The results with lmoments3 (PWM) / gamma is close to what we get with ML/gamma or APP/gamma, so I'm satisfied, things work well. |
| if method == "PWM": | ||
| lmom = pytest.importorskip("lmoments3.distr") | ||
| scipy2lmom = {"gamma": "gam"} | ||
| dist = getattr(lmom, scipy2lmom[dist]) |
There was a problem hiding this comment.
@Zeitsperre are we sure that any *-lmoments3 tox test suite is running here ?
@coxipi Because I think the test should fail : dist is passed as a lmoments3 object in the test but in the SPI and SPEI functions there are tests like dist in dist_methods, which would fail. I'm not sure how to do it, but the function/indicator must now be able to accept scipy/lmoments3 distribution objects if the method=='PWM' is to be enabled!
There was a problem hiding this comment.
Ah you're right! in the tests I have called the low-level function, so it's really params that counts in the SPI call. The default distribution that is sent in the condition checker does not matter, so that's why things worked out. But the SPI/SPEI cannot truly accept PWM functions yet.
Perhaps I should add a dictionnary to convert scipy names to lmoments3 names.
Or, simply: Since we don't want to officially support lmoments3, we can just forget about the conditions if method="PWM" ? We allow all distributions by default with PWM
Pull Request Checklist:
number) and pull request (:pull:number) has been addedWhat kind of change does this PR introduce?
fitkwargsis given withlmomentsmethodgammadistribution is used withfloc: This is accepted and used to shift the distributionDoes this PR introduce a breaking change?
No
Other information:
I realize that SPI/SPEI is still heavily constrained in what distribution can be used. For now, only gamma/fisk (log-logistic) are allowed. At some point we could revisit this. I think we reached a point where we could be more permissive, since we ironed out many of the potential problems in these functions. There is still potential that problems could arise if we allow more distributions, but we might have to accept that some problems might arise when working with these optimization procedures, and fix them on the fly (or not, if it's just a limitation of the method).