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

DEP: interpolate: deprecate interp2d #17180

Merged
merged 6 commits into from
Nov 6, 2022
Merged

Conversation

ev-br
Copy link
Member

@ev-br ev-br commented Oct 9, 2022

Reference issue

mailing list discussion: https://mail.python.org/archives/list/scipy-dev@python.org/thread/LU72CAGYFH37PZWCANMUF6UWFNSPZRHU/

What does this implement/fix?

Make interp2d raise DeprecationWarnings starting from scipy 1.10

Additional information

Posting early to increase exposure: the mailing list thread needs at least two weeks (i.e., this can go in not earlier than Oct 20).
Also the transition guide link is for now to a github gist, will likely need to be updated based on the final state of gh-17178.

@ev-br ev-br added scipy.interpolate deprecated Items related to behavior that has been deprecated labels Oct 9, 2022
@ev-br
Copy link
Member Author

ev-br commented Oct 9, 2022

@j-bowhay Jake, would you mind pointing me to how to keep track of the deprecation? Especially given the three-step process, as suggested by Matthew in the mailing list?

@ev-br ev-br added this to Ready for review in scipy.interpolate Oct 9, 2022
@ev-br ev-br added this to the 1.10.0 milestone Oct 9, 2022
@j-bowhay
Copy link
Member

j-bowhay commented Oct 9, 2022

The status of all deprecation if currently tracked in #15765 cc. @h-vetinari

Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

A few comments to hopefully make this more consistent with other deprecations. Also I think it would be nice to have a test checking that the warning appears when it is expected to.

scipy/interpolate/_interpolate.py Outdated Show resolved Hide resolved
scipy/interpolate/_interpolate.py Outdated Show resolved Hide resolved
scipy/interpolate/_interpolate.py Outdated Show resolved Hide resolved
@j-bowhay
Copy link
Member

Hmm documentation not rendering quite right. Not sure if it's the new lines in the message or an issue with the indentation that's causing the problem.
image

@ev-br
Copy link
Member Author

ev-br commented Oct 11, 2022

OK, I think it's easiest to just copy-paste the message into the docstring. Will do together with eating-own-dogfood exercise, there is interp2d usage in scipy.stats to be ported over.

@ev-br
Copy link
Member Author

ev-br commented Oct 11, 2022

Test failures are due to internal interp2d usage in scipy.stats, which gh-17198 removes.

ev-br and others added 5 commits October 14, 2022 08:20
Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
Instead of fiddling with string substitution and related indenting,
just copy the message into the docstring and be done with it.
@ev-br
Copy link
Member Author

ev-br commented Oct 14, 2022

Rebased on main now that gh-17198 is in, and lumped together review suggestions from Jake.

Apologies for the mass ping of code owners! This was a git mistake when pulling in changes made in the GH interface.

Recording the error here: apparently, when there is a review change via GH interface "suggested edit" and a local change, updating a PR needs two rebases: one origin/<PR fork> to pull in the reviewer edits, and the second on upstream/main to sync the PR branch with the upstream main.

scipy/interpolate/_interpolate.py Outdated Show resolved Hide resolved
scipy/interpolate/_interpolate.py Outdated Show resolved Hide resolved
@ev-br ev-br moved this from Ready for review to In review in scipy.interpolate Oct 14, 2022
Copy link
Member

@j-bowhay j-bowhay left a comment

Choose a reason for hiding this comment

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

Other than the lint errors this looks good to go now

@ev-br ev-br force-pushed the deprecate_interp2d branch 2 times, most recently from e815f5d to 9ded68d Compare October 16, 2022 12:13
@ev-br
Copy link
Member Author

ev-br commented Oct 31, 2022

No objections surfaced in the ML discussion, so I'd like to merge this for 1.10 before branching. Am going to hit the merge button this week unless further comments.

One remaining glitch is that the porting recommendations are still in a gist under my GH account. This can be adjusted in a follow-up though, if we figure a better place for it.

@ev-br
Copy link
Member Author

ev-br commented Nov 6, 2022

OK, the mailing list discussion outcome is positive, and there are no further comments either in the ML or here. Let's press on with this deprecation. Thanks Jake for the help, thanks to everybody who contributed to the ML discussion. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated Items related to behavior that has been deprecated scipy.interpolate
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants