xclim
xclim copied to clipboard
Ouranosinc
Add a PWM option for frequency analysis?
Generic Issue
I've been using your very useful xclim.indices.stats.frequency_analysis function.
By default that function implements the maximum-likelihood (ML) fitting method. The actual fit function (xclim.indices.stats.fit) has the option of using the probability weighted moments (PWM) fitting method instead, so I was wondering if it would be possible to add a method option to xclim.indices.stats.frequency_analysis so that the user can choose ML or PWM?
I'd be happy to help implement this change, but I wondered if perhaps there was a technical reason why it isn't possible to add a method option to xclim.indices.stats.frequency_analysis?
Code of Conduct
- [X] I agree to follow this project's Code of Conduct
Hi @DamienIrving,
I don't believe there's a technical reason why we haven't implemented it. Often the indices/indicators that get the most attention are the ones we rely on for our internal/external projects. We would be more than happy to accept a PR if you wish to implement this method.
I'm not the statistician (@huard or @aulemahal might be more opinionated here) but in order to prevent breaking changes, my only advice would be to ensure that "ml" be the default behaviour of the indice.
Be sure to follow the contributing guidelines. Cheers!
The arguments against this would be:
- PWM relies on lmoments3, which is not actively maintained. For this reason, xclim considers it an optional dependency.
- Adding this option slightly complicates the user interface (not all distributions have a PWM fit implementation, and this needs to be explained in the docstring).
So it's possible to make the changes, I'm not against it, but I don't want to fragilise the high-level functions as a result. Suggestions welcome.
FYI: Ouranosinc is now responsible for lmoments3 development and maintenance, and the package will be a core dependency of xclim>=0.42.0. David's first point concerning PWM and lmoments3 has been resolved.