-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
MAINT: remove incidental imports from private modules #19904
Conversation
These imports were intentional; they were part of the plan to remove these unnecessary imports (and other pseudo-public attributes) in a backward compatible way. The current behavior is, e.g.: from scipy.optimize.slsqp import exp which produces the desired warning message.
The tests checking this behavior are failing. But I guess they can be removed now too since we said they'd be removed in SciPy 1.13.0. |
In one of these discussions the reason given for keeping these imports was that they once were part of
In any case, we've scheduled them for removal in 1.13, so the timing is still OK. 🙃 |
So yeah, go ahead and remove the tests. It's fine with me to take all these out now, even if 1.13 is coming earlier than usual. |
Actually, we've delayed this to 1.14 in #19892. So I'll wait until 1.13 is out. |
34972e4
to
a916e13
Compare
5ef5259
to
41c5b64
Compare
@j-bowhay this is mainly a follow-up to your efforts around removing those incidental imports - I think it's ready and would appreciate your review (I think we were close already a month ago; I suggest to finish this now). |
Both this and #19673 are very much still on my radar I just need to finish a few exams and assignments before I can spend more than 10 minutes looking at something! I will get to this in May which I assume is fine with the 1.14 timeline. |
Friendly ping @h-vetinari as I am keen to get this in before branching for the next release. Can you resolve the merge conflicts? |
It looks like you've accidentally picked up a submodule change |
While I generally ask not to, this PR can (or even: should) be squash-merged. |
The failure here is unrelated:
|
Any further comments here @j-bowhay et al.? 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thanks for tackling @h-vetinari
Thanks for reviews & merging! |
This has now grown into a full-blown fix for #19905; while we're targetting 1.14 with this, I wanted to check if we're actually warning in all cases (after discovering some potential discrepancies).
Previously:
Some more follow-ups from the work for #18279, same as #19893.
I've tried & failed to import these on 1.11 resp 1.12; the imports are fully ignored AFAICT due to the presence of
__all__
in those private modules.Not sure what the intention there was @j-bowhay, but in any case, this should be good to remove IMO.