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: replace interp2d by stub #20497

Merged
merged 9 commits into from
May 25, 2024
Merged

Conversation

h-vetinari
Copy link
Member

Towards #18583, details & references in that issue.

@h-vetinari h-vetinari added scipy.interpolate deprecated Items related to behavior that has been deprecated labels Apr 16, 2024
@h-vetinari h-vetinari requested a review from ev-br April 16, 2024 23:40
@h-vetinari
Copy link
Member Author

The last commit here can be omitted if we merge #19904 first.

@h-vetinari h-vetinari mentioned this pull request Apr 17, 2024
29 tasks
@h-vetinari
Copy link
Member Author

So now we have the issue that the transition guide wants to execute interp2d, which is obviously not possible anymore. Should we simply delete the guide and in the error/docstring refer to the 1.13 transition guide?

@ev-br
Copy link
Member

ev-br commented Apr 17, 2024

This is an interesting fallout :-). Basically, this page should be static. And well discoverable, esp after interp2d starts raising hard errors.
Whether it's back to ReST or link to 1.13 docs --- whichever works.

@j-bowhay j-bowhay added the needs-work Items that are pending response from the author label Apr 19, 2024
@h-vetinari
Copy link
Member Author

Is there an easy way to translate the current website in main to a static RST/MD file, where we don't re-execute the removed import? CC @tupui

@j-bowhay
Copy link
Member

Could we move the offending code cells to be markdown cells with the content surrounded with backticks and then just put the images in?

@tupui
Copy link
Member

tupui commented Apr 29, 2024

Is there an easy way to translate the current website in main to a static RST/MD file, where we don't re-execute the removed import? CC @tupui

I am not sure what you mean. If it's about the API doc itself, I am not sure you can generate it and save it for later builds (besides the normal caching from Sphinx, but I never managed to get this caching working in our CI, I had 2-3 PRs trying various things and gave up. Yes this is 💯 ridiculous that we have to do 20 mins runs for the doc on all PRs 😅😭).

@h-vetinari
Copy link
Member Author

Could we move the offending code cells to be markdown cells with the content surrounded with backticks and then just put the images in?

Wouldn't we have to turn it from a notebook into a regular markdown page? I guess we could save the images from a local run with 1.13 (or main).

I am not sure what you mean.

It might go through many layers, but the end result is a static HTML page. If we can keep showing that webpage, the goal would be achieved; preferably without stripping off too many layers of abstraction.

However I'm tending towards just referring to the 1.13 docs. Those will continue to be a notebook that works fine, and it avoids a large effort for (IMO) questionable gain.

@lucascolley lucascolley changed the title DEP: replace interp2d by stub DEP: interpolate: replace interp2d by stub May 17, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a pity to delete our only current executable notebook, but it's too much of a hassle for me to work around wanting to execute functions that don't exist anymore. I won't stand in the way of anyone wanting to come up with a better solution, but - absent further input how this could/should be done - for now I'm going to just refer to the 1.13 transition guide.

@h-vetinari h-vetinari added this to the 1.14.0 milestone May 18, 2024
@ev-br
Copy link
Member

ev-br commented May 18, 2024

cc @melissawm for vis

@@ -82,8 +82,6 @@ These can be opened as Jupyter Notebooks with the help of the
.. toctree::
:caption: Executable tutorials
:maxdepth: 1

../notebooks/interp_transition_guide
Copy link
Member

Choose a reason for hiding this comment

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

For better visibility maybe we should do

interp2d transition guide <https://docs.scipy.org/doc/scipy-1.13.0/notebooks/interp_transition_guide.html>

@melissawm
Copy link
Contributor

Hi all, I am happy to help in the transition here - in this case I do think the best option is to transform this into a static page, either md or rst (md is easier). For information, we will likely have other executable notebooks added in the near future (see #20303)

@h-vetinari
Copy link
Member Author

Hi all, I am happy to help in the transition here - in this case I do think the best option is to transform this into a static page, either md or rst (md is easier).

Thanks for the offer to help - I'm going to need it! 😅

No time to dig into the conversion to a static website, but if that doesn't waste a lot of your time, please go for it! You can commit directly into this PR if you want. :)

@melissawm
Copy link
Contributor

melissawm commented May 23, 2024

This should do it - let me know if it looks reasonable. I'd love for someone to check that the alt-text in the images makes sense 🙏🏻 I don't think describing more details than what I did is useful as it's more logical that a screenreader user will expect the results to be the output of the computation described in the page. Happy to hear other opinions, though!

Rendered page: https://output.circle-artifacts.com/output/job/d64b363b-5601-42a2-b5af-5b76aa7c8ac2/artifacts/0/html/tutorial/interpolate/interp_transition_guide.html

I also moved it to be in the main ToC for the Interpolate tutorial.

@melissawm
Copy link
Contributor

Not quite sure why refguide_check is complaining about .. versionremoved:: since we are using Sphinx 7.3.7 and it should definitely support this (newly added) directive.

@tupui
Copy link
Member

tupui commented May 23, 2024

Not quite sure why refguide_check is complaining about .. versionremoved:: since we are using Sphinx 7.3.7 and it should definitely support this (newly added) directive.

I think you need to add the directive in the refguide_check.py in ok_unknown_items

@h-vetinari
Copy link
Member Author

Thank you so much! 😊

I just added a small fix on top - the docstring was still referring to the 1.13 version (was hard to see in the diff due to other removals), and I think that the error-message should not use a relative page reference, but contain a URL that users can copy & paste without having to search for that page.

@ev-br
Copy link
Member

ev-br commented May 24, 2024

While we're at it, it'd be great to also address #20620. Can do in a follow-up though.

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.

LGTM

@j-bowhay j-bowhay removed the needs-work Items that are pending response from the author label May 24, 2024
@h-vetinari
Copy link
Member Author

While we're at it, it'd be great to also address #20620. Can do in a follow-up though.

That's to me a very different direction (clean up content/docs and/or decide to align defaults somehow), so I wouldn't want to couple this here, please.

@j-bowhay
Copy link
Member

@h-vetinari are you happy for this to be squash merged?

@h-vetinari
Copy link
Member Author

Yes! Thanks for asking :)

@h-vetinari h-vetinari merged commit 63f4f73 into scipy:main May 25, 2024
31 checks passed
@h-vetinari h-vetinari deleted the interp2d branch May 25, 2024 08:52
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants