Skip to content
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

indices.stats.fit fails for PWM with less than dist.numargs +2 values #1849

Closed
2 tasks done
saschahofmann opened this issue Jul 22, 2024 · 2 comments · Fixed by #1850
Closed
2 tasks done

indices.stats.fit fails for PWM with less than dist.numargs +2 values #1849

saschahofmann opened this issue Jul 22, 2024 · 2 comments · Fixed by #1850
Labels
bug Something isn't working

Comments

@saschahofmann
Copy link
Contributor

saschahofmann commented Jul 22, 2024

Setup Information

  • Xclim version: 0.51.0
  • Python version: 3.11.1
  • Operating System: Ubuntu

Description

Using the fit function with the PWM method and a lmoments3 dist fails if the dataarray contains slices with less than dist.numargs + 2 non-null values (see below for an reproducible example). The simple solution would be to add another check in _fitfunc_1d when using the PWM method. I already implemented a monkeypatch for us so will provide a PR in a sec.

Steps To Reproduce

from xclim.indices.stats import _fitfunc_1d
import xarray as xr
import lmoments3.distr
import numpy as np

da = xr.DataArray([3,np.nan,1,np.nan], coords=[('x',[1,2,3,4])])
dist = lmoments3.distr.gum
_fitfunc_1d(da, dist=dist, method='PWM', nparams=3)

fails with

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
python3.11/site-packages/xclim/indices/stats.py:51)     args, kwargs = _fit_start(x, dist.name, **fitkwargs)
python3.11/site-packages/lmoments3/distr.py:66) elif not lmom_ratios:

ValueError: At least 2 data points must be provided.

Additional context

No response

Contribution

  • I would be willing/able to open a Pull Request to address this bug.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@aulemahal
Copy link
Collaborator

I'm not sure I fully understand the issue : why wouldn't we want to raise an error here ? If this call is invalid with lmoments3 shouldn't the user be made aware of it ?

@saschahofmann
Copy link
Contributor Author

I found that for larger DataArrays it's quite difficult to figure out why the call is failing for one and not for another since this is often done slices of the data. For all-nan slices we already return NaNs which makes sense, I guess we could raise a warning for affected slices or have a setting that like fail_on_too_few_args ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants