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.

Sign up for GitHub

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

Jump to bottom

MAINT: optimize.isotonic_regression: remove unnecessary copies #20582

Merged
merged 4 commits into from
May 28, 2024
Merged

MAINT: optimize.isotonic_regression: remove unnecessary copies #20582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
595be62
ENH unweighted isotonic_regression
lorentzenchr Apr 25, 2024
630aa6d
FIX scalar input
lorentzenchr Apr 26, 2024
109e457
TST adapt test for allowed scalars
lorentzenchr Apr 26, 2024
3f2ac49
MNT use atleast_1d
lorentzenchr May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions scipy/optimize/_isotonic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,13 @@ class of strictly consistent scoring functions for the mean, see [2]_
input y of length 1000, the minimizer takes about 4 seconds, while
``isotonic_regression`` takes about 200 microseconds.
"""
yarr = np.asarray(y) # Check yarr.ndim == 1 is implicit (pybind11) in pava.
yarr = np.atleast_1d(y) # Check yarr.ndim == 1 is implicit (pybind11) in pava.
order = slice(None) if increasing else slice(None, None, -1)
x = np.array(yarr[order], order="C", dtype=np.float64, copy=True)
lorentzenchr marked this conversation as resolved.
Show resolved Hide resolved
if weights is None:
warr = np.ones_like(yarr)
wx = np.ones_like(yarr, dtype=np.float64)
lorentzenchr marked this conversation as resolved.
Show resolved Hide resolved
else:
warr = np.asarray(weights)
warr = np.atleast_1d(weights)

if not (yarr.ndim == warr.ndim == 1 and yarr.shape[0] == warr.shape[0]):
raise ValueError(
Expand All @@ -134,9 +136,7 @@ class of strictly consistent scoring functions for the mean, see [2]_
if np.any(warr <= 0):
raise ValueError("Weights w must be strictly positive.")

order = slice(None) if increasing else slice(None, None, -1)
x = np.array(yarr[order], order="C", dtype=np.float64, copy=True)
wx = np.array(warr[order], order="C", dtype=np.float64, copy=True)
wx = np.array(warr[order], order="C", dtype=np.float64, copy=True)
n = x.shape[0]
r = np.full(shape=n + 1, fill_value=-1, dtype=np.intp)
x, wx, r, b = pava(x, wx, r)
Expand Down
4 changes: 3 additions & 1 deletion scipy/optimize/tests/test_isotonic_regression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ class TestIsotonicRegression:
"Input arrays y and w must have one dimension of equal length"),
([0, 1], [1],
"Input arrays y and w must have one dimension of equal length"),
(1, 2,
(1, [1, 2],
"Input arrays y and w must have one dimension of equal length"),
([1, 2], 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to change the regression tests in a PR that only aims to avoid extra array copies? If Evgeni is happy with this I don't mind, but why do we need to get rid of the old test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because isotonic_regression(y=1, weights=2) should work. In current main branch, the scalar values are not promoted to arrays of shape (1,) and an error is raised.
With this PR they are reshaped and no error is raised anymore.
Passing scalar values is an edge case because the solution is trivial (return the same values), and no proper usage of isotonic regression will/should do that.

"Input arrays y and w must have one dimension of equal length"),
([0, 1], [0, 1],
"Weights w must be strictly positive"),
Expand Down