Skip to content

Conversation

mdhaber
Copy link
Contributor

@mdhaber mdhaber commented Feb 2, 2022

Reference issue

closes gh-15507
closes gh-14471
closes gh-14081
addresses linprog part of gh-13846
closes gh-13200
closes gh-12485
closes gh-11617
closes gh-11570
closes gh-11515
closes gh-11326
closes gh-11215
closes gh-11201
closes gh-10288
closes gh-9671
closes gh-9536
closes gh-7877

I don't think GitHub will take care of PRs automatically:
closes gh-14488
closes gh-12510
closes gh-11618

What does this implement/fix?

The HiGHS methods for linprog have been thoroughly tested in the last few SciPy releases and have demonstrated that they are (much) faster and more robust than the original methods. This PR makes method='highs' the default and deprecates method='simplex', method='revised simplex', and method='interior-point'.

Additional information

  • We need to send an email to the mailing list before this is merged.

@mdhaber mdhaber requested review from mckib2 and Kai-Striega February 2, 2022 18:58
@mdhaber mdhaber requested a review from andyfaff as a code owner February 2, 2022 18:58
@mdhaber mdhaber added scipy.optimize maintenance Items related to regular maintenance tasks labels Feb 2, 2022
Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

I am +1 on reducing our technical debt especially if we have better methods.

@lorentzenchr
Copy link
Contributor

In scikit-learn's QuantileRegressor, we repeatedly observed the superiority of the HiGHS methods. Therefore, I would welcome the proposed change.

Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for taking the time to tidy this up.

@mdhaber
Copy link
Contributor Author

mdhaber commented Feb 3, 2022

Great. Let's give it a week for comments and merge on 2/9 if there are no objections.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Comment applies in several places of the diff.

Comment on lines +410 to +411
`'simplex'`) are legacy methods and will be removed in the second release
after SciPy 1.9.0.
Copy link
Member

Choose a reason for hiding this comment

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

"second release after SciPy 1.9.0." is not clear - presumably this doesn't refer to 1.9.2, but 1.11.0? Unless there are concrete plans for a scipy 2.0, I'd just write 1.11.0 here, or at least qualify "second non-patch release after 1.9.0".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks... yeah, it would have been "1.11.0" but for comments in gh-13490. I was thinking about qualifying with "minor" but was worried some users would think that would mean bug-fix release. Is there a standard description other than "non-patch"? "feature" release?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I looked at that discussion, but I kinda fail to see the reason why the minor counter couldn't go to two digits - numpy and cpython do the same...? I'm thinking a putative 2.0 should come with some degree of "large change compared to 1.x", even if it isn't breaking anything**.

In any case, I'd say it would be simpler to just leave 1.11 here, and if a version 2.0 is decided (should we open an issue to discuss?), then it's a simple matter of searching for "1.10"/"1.11"/etc. and replacing them appropriately.

CC @ev-br @rgommers @tupui @tylerjereddy

Is there a standard description other than "non-patch"? "feature" release?

I wanted to avoid "minor" due to not mapping well to scipy's versioning scheme, but "feature release" would work. That's still worse than using 1.11 IMO, but definitely an improvement over what we have now.

** as an example in terms of the kind of scope for a major bump that I have in mind, a current(ly-talked-about) candidate for cpython 4.0 is if/once the GIL is removed. On the numpy side, although there is an ancient, non-empty milestone, I'm not aware of current plans for 2.0 (though I did once wager that it'll come about eventually 😉) - comprehensively fixing integer promotion issues might be a candidate to go to numpy 2.0.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say "two releases after N" and leave it at that

Copy link
Member

Choose a reason for hiding this comment

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

I'd say "two releases after N" and leave it at that

That's the part I disagree with. Strictly speaking "two releases after 1.9.0" is "1.9.2", which is clearly not what is intended here.

Copy link
Member

Choose a reason for hiding this comment

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

If you feel strongly enough to conflate the issue at hand with a decision on the 1.10 vs 2.0 naming, I suggest to separate the latter and discuss it on the mailing list.

My personal opinion is that 2.0 is no special and I'd have a slight preference for 1.9->2.0. That said, it's not quite my hill so I'd go with a majority on this one :-)

Copy link
Member

@h-vetinari h-vetinari Feb 5, 2022

Choose a reason for hiding this comment

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

Respectfully, I'm not conflating anything. Avoiding a direct "will be removed in Scipy 1.11.0" and ending up with awkward wording (i.e. the issue at hand) only makes sense if a possible 2.0 release is a possibility. Still, I'll open an issue to discuss the version after 1.9.

Copy link
Member

Choose a reason for hiding this comment

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

Still, I'll open an issue to discuss the version after 1.9.

#15528

@h-vetinari h-vetinari added this to the 1.9.0 milestone Feb 5, 2022
Copy link
Contributor

@mckib2 mckib2 left a comment

Choose a reason for hiding this comment

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

Looks great! CC'ing HiGHS upstream for awareness @jajhall

@mdhaber
Copy link
Contributor Author

mdhaber commented Feb 8, 2022

@tupui @mckib2 @Kai-Striega @ev-br I sent the message to the mailing list about one week ago. Would one of you like to merge this tomorrow if no objections come up before then?

@h-vetinari I agree "two releases" is more likely to be interpreted as two bug-fix releases. Let's going ahead and merge this as-is and I'll come back and fix this and the gh-13490 wording once gh-15528 is resolved. Sound good?

@tupui
Copy link
Member

tupui commented Feb 9, 2022

@tupui @mckib2 @Kai-Striega @ev-br I sent the message to the mailing list about one week ago. Would one of you like to merge this tomorrow if no objections come up before then?

@h-vetinari I agree "two releases" is more likely to be interpreted as two bug-fix releases. Let's going ahead and merge this as-is and I'll come back and fix this and the gh-13490 wording once gh-15528 is resolved. Sound good?

Happy to do the honour tomorrow 😉

@tupui tupui merged commit d119ea4 into scipy:main Feb 9, 2022
@tupui
Copy link
Member

tupui commented Feb 9, 2022

Thanks @mdhaber, quite a few issue closed 😅👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.optimize
Projects
None yet
7 participants