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

ENH use Scipy isotonic_regression #28897

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

Closes #27378.

What does this implement/fix? Explain your changes.

Use scipy.optimize.isotonic_regression as of scipy 1.12 instead of our own _inplace_contiguous_isotonic_regression.

Any other comments?

@lorentzenchr lorentzenchr changed the title Scipy isotonic ENH use Scipy isotonic_regression Apr 26, 2024
Copy link

github-actions bot commented Apr 26, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 4ca6e8b. Link to the linter CI: here

@lorentzenchr
Copy link
Member Author

Within uncertainty, this shows no performance regression:

This PR

% python benchmarks/bench_isotonic.py --iterations 10 --log_min_problem_size 2 --log_max_problem_size 8 --dataset perturbed_logarithm

100 0.0004406755011586938
1000 0.00043975089865853076
10000 0.00060384689932107
100000 0.0022764566019759513
1000000 0.02123089429878746
10000000 0.1800829847015848
% python benchmarks/bench_isotonic.py --iterations 10 --log_min_problem_size 2 --log_max_problem_size 8 --dataset pathological
100 0.0004570792989397887
1000 0.0004778172013175208
10000 0.0008545864999177866
100000 0.005397420399094699
1000000 0.06050054539737175
10000000 1.0579413614992519

MAIN

% python benchmarks/bench_isotonic.py --iterations 10 --log_min_problem_size 2 --log_max_problem_size 8 --dataset perturbed_logarithm
100 0.00048800000000000004
1000 0.0004021
10000 0.0005796000000000001
100000 0.0023363999999999998
1000000 0.019411199999999997
10000000 0.1722571
% python benchmarks/bench_isotonic.py --iterations 10 --log_min_problem_size 2 --log_max_problem_size 8 --dataset pathological
100 0.0004615
1000 0.00044409999999999995
10000 0.0008770999999999999
100000 0.005215
1000000 0.05959310000000001
10000000 0.9408343

thomasjpfan
thomasjpfan previously approved these changes May 4, 2024
y = np.array(y[order], dtype=y.dtype)
sample_weight = _check_sample_weight(sample_weight, y, dtype=y.dtype, copy=True)
sample_weight = np.ascontiguousarray(sample_weight[order])
if sp_version >= parse_version("1.12.0"):
Copy link
Member

Choose a reason for hiding this comment

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

Typically, we add this type of branching in utils.fixes. Although, given the scope, I am okay with this approach.

Nit: It's a little bitter to compare base versions:

Suggested change
if sp_version >= parse_version("1.12.0"):
if sp_base_version >= parse_version("1.12.0"):

@thomasjpfan thomasjpfan dismissed their stale review May 4, 2024 14:30

Looks like related tests are failing

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

The CI failures look related to isotonic_regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace _inplace_contiguous_isotonic_regression by scipy.optimize.isotonic_regression
2 participants