Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

ENH use Scipy isotonic_regression #28897

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
Merged
merged 5 commits into from
May 15, 2024
Merged

ENH use Scipy isotonic_regression #28897

merged 5 commits into from
May 15, 2024

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
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 26, 2024 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 43cadcb. 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
View reviewed changes
sklearn/isotonic.py Outdated Show resolved Hide resolved
@thomasjpfan thomasjpfan dismissed their stale review May 4, 2024 14:30

Looks like related tests are failing

thomasjpfan
thomasjpfan reviewed May 9, 2024
View reviewed changes
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.

@lorentzenchr
Copy link
Member Author

lorentzenchr commented May 10, 2024 •
edited
Loading

The CI failures look related to isotonic_regression.

I see that. But I have no clue how that happens. Scipy's isotonic_regression is thoroughly tested. And I can't reproduce the test failures locally. See 43cadcb

@lorentzenchr lorentzenchr force-pushed the scipy_isotonic branch 3 times, most recently from 3e903c3 to fb793e7 Compare May 10, 2024 09:24
@lorentzenchr
Copy link
Member Author

@thomasjpfan CI is green.

thomasjpfan
thomasjpfan approved these changes May 10, 2024
View reviewed changes
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.

LGTM

OmarManzoor
OmarManzoor approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @lorentzenchr

@OmarManzoor OmarManzoor merged commit 5c28a8e into scikit-learn:main May 15, 2024
30 checks passed
@lorentzenchr lorentzenchr deleted the scipy_isotonic branch May 15, 2024 15:30
@jeremiedbb jeremiedbb mentioned this pull request May 20, 2024
Merged
14 tasks
@jeremiedbb jeremiedbb mentioned this pull request Jul 2, 2024
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees
No one assigned
Labels
No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace _inplace_contiguous_isotonic_regression by scipy.optimize.isotonic_regression
3 participants
@lorentzenchr @thomasjpfan @OmarManzoor
Morty Proxy This is a proxified and sanitized view of the page, visit original site.