Fewer checks on xdata for curve_fit. #10196
Conversation
There was a problem hiding this comment.
I'll let the resident optimize experts chime in with more meaningful comments, but I just did a quick pass through the line coverage data in Azure in case that is helpful.
Not sure it is crucial to add more tests for what sounds like a simple-enough adjustment though. Probably a good sign that no old tests have been modified, and just a new test added.
| else: | ||
| ydata = np.asarray(ydata) | ||
| ydata = np.asarray(ydata, float) |
There was a problem hiding this comment.
this code path is never hit by a test
There was a problem hiding this comment.
But this was already the case before, wasn't it?
There was a problem hiding this comment.
Yes, my point is that confidence & safety in code changes drops for lines that aren't covered by tests.
There was a problem hiding this comment.
I'm not really sure how you would go about testing this, apart from trying to fit a dataset with a different dtype and seeing if it failed.
There was a problem hiding this comment.
I modified another test to exercise that code path.
| else: | ||
| xdata = np.asarray(xdata) | ||
| xdata = np.asarray(xdata, float) |
|
|
||
| if isinstance(xdata, (list, tuple, np.ndarray)): | ||
| # `xdata` is passed straight to the user-defined `f`, so allow | ||
| # non-array_like `xdata`. | ||
| if check_finite: | ||
| xdata = np.asarray_chkfinite(xdata) | ||
| xdata = np.asarray_chkfinite(xdata, float) |
There was a problem hiding this comment.
My comment doesn't belong here, but still...
The docstring for xdata needs to be updated.
There was a problem hiding this comment.
I think it needs to be explicit.
xdata : array_like or None
There was a problem hiding this comment.
It can really be any object, so I edited the docstring accordingly.
anntzer
force-pushed
the
curve_fit-xdata
branch
2 times, most recently
from
7188e68
to
273029a
Compare
The *xdata* argument to curve_fit is essentially a helper in the case
where one wants to write the fit function as `func(x, *params)`. Until
scipy 0.13, it was possible to just pass a dummy value (e.g. None) as
*xdata*, e.g.
```
curve_fit(lambda _, a: a * np.arange(10), None, 2 * np.arange(10))
```
A comment in the implementation clearly states that the intent was for
*xdata* to be anything, including a non-array:
```
if isinstance(xdata, (list, tuple, np.ndarray)):
# `xdata` is passed straight to the user-defined `f`, so allow
# non-array_like `xdata`.
```
(Effectively, this is an interface closer to the one of `least_squares`,
with the advantage that the covariance matrix is also returned.)
0.13.0 added two checks that make this impossible: it now requires that
*xdata* is not empty, and casts it to float64 for improved precision.
The non-emptiness check is spurious: it was introduced at the same time
as a check that *ydata* not be empty (which is necessary to avoid a
confusing error message, and is reasonable). This PR removes it.
The cast to float64 can be applied only if xdata is indeed an
array-like, which this PR does.
|
I'm happy with these changes. Could you add a line to the release notes outlining what's changed here? |
|
Is there any chance this could go to 1.3.1 (if there are plans for such a release) instead of 1.4, on the basis that this fixes a regression? If yes, there's no wiki page for 1.3.1 release notes. |
|
I've marked it as having a 1.3.1 milestone, hopefully that's the mechanism for specifying that it should be added to such a release, if it occurs. |
|
I'll just post the release notes entry here, you can move it to whereever appropriate :)
|
tylerjereddy
added
the
backport-candidate
|
@andyfaff It is possible to do 1.3.1 I think, although I'd suggest that we haven't quite reached the "critical mass" of backports to justify it yet. I'll put the backport label on as well just in case. |
rgommers
removed
the
backport-candidate
|
This regression came back, and rather than fixing it, the docs were changed. See: #12632 |
The xdata argument to curve_fit is essentially a helper in the case
where one wants to write the fit function as
func(x, *params). Untilscipy 1.3.0, it was possible to just pass a dummy value (e.g. None) as
xdata, e.g.
A comment in the implementation clearly states that the intent was for
xdata to be anything, including a non-array:
(Effectively, this is an interface closer to the one of
least_squares,with the advantage that the covariance matrix is also returned.)
1.3.0 added two checks that make this impossible: it now requires that
xdata is not empty, and casts it to float64 for improved precision.
The non-emptiness check is spurious: it was introduced at the same time
as a check that ydata not be empty (which is necessary to avoid a
confusing error message, and is reasonable). This PR removes it.
The cast to float64 can be applied only if xdata is indeed an
array-like, which this PR does.
xref #9893, #10076.