-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add errors option to curvefit #7891
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
Conversation
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.
Looks good, thanks for adding a test for the original PR.
Just some minor remarks
xarray/core/dataarray.py
Outdated
allow_failures: bool, default: False | ||
If True and the underlying `scipy.optimize_curve_fit` optimization fails for | ||
any of the fits, return NaN in coefficients and covariances for those | ||
coordinates. Helpful when fitting multiple curves and some of the data just |
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.
Remove the Helpful... Part. It just adds clutter for little benefit.
If you want to add such tips do it above the argument list.
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.
Done in d3decbb. I was also wondering if the name could be something different, like allow_errors
for example. Are there any other examples of similar args in xarray, so we could be consistent with those?
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.
Good point.
In other places we use errors = "raise" | "ignore" | "warn"
(the warn part is optional)
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.
Okay, I'll change it to that.
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.
Fixed in efce0b3. I was considering adding the option to also warn, but looks like scipy has its own internal mechanism for producing warnings, see here: https://github.com/scipy/scipy/blob/v1.10.1/scipy/optimize/_minpack_py.py#L491. I'm not sure how to control it and didn't want to get into that now. Maybe a warning is now always issued? Although I didn't see any warnings in pytest on my setup at least.
Oh no, the doctest failure is because the test is flaky, this was introduced by me in #7821, see here: #7821 (comment) and here: a0e6659. I'll submit another patch to fix it soon, although I'm not sure how. If you have any tips to avoid this problem, let me know. |
You could use |
Better to use a tolerance in the assterion testing. |
* Add allow_failures flag to Dataset.curve_fit * Reword docstring * Add allow_failures flag also to DataArray * Add unit test for curvefit with allow_failures * Update whats-new Co-authored-by: Dominik Stańczak <[email protected]> * Add PR to whats-new * Update docstring * Rename allow_failures to errors to be consistent with other methods * Compute array so test passes also with dask * Check error message * Update whats-new --------- Co-authored-by: Dominik Stańczak <[email protected]> Co-authored-by: Deepak Cherian <[email protected]>
xarray.curve_fit
#6317 and closes Add allow_failures flag to Dataset.curve_fit #6515whats-new.rst
This is a rebased version of #6515, with the arg
errors = "raise" | "ignore"
added toDataset
andDataArray
, and with tests. Let me know if the tests should be expanded further.