-
-
Notifications
You must be signed in to change notification settings - Fork 188
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: Handle warnings in tutorials #203
Conversation
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.
We need to deal with the oldest supported scipy version, otherwise this looks good to me.
Good point - though I think there's a broader discussion to be had here. IMO the most important thing is supporting the latest releases. I'm not sure there's as much value in supporting the oldest devs. |
I'm thinking mostly about end users who may stuck on older systems without the ability (or in fact the need) to upgrade versions to try to run a tutorial. Not sure how much this is a real concern. |
Yeah that's a good point, and it jives with some of the other unofficial policies that the tutorials have adopted; e.g. not using sphinx-specific features (like intersphinx) in the tutorials themselves so they're still 100% functional as standalone notebooks. There's definitely a wider discussion to be had :) |
What about getting in the try/except to make the current CI setup happy, and then discussing what would be sensible to do down the road? |
289b20d
to
860ba4a
Compare
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.
Now it all looks good, thank you!
Maybe open an issue about the minimum scipy version, etc policies, so we don't forget to actually have the discussion :)
content/x_y-squared.npz
Outdated
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.
Hmm, this file is created by the notebook and then read back, so don't add it to the repo.
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.
Yeah, I must've caught it in a git commit -a
. The file is already tracked, but it definitely shouldn't be. I'll make sure to back out the "changes" to the file here. We should remove it entirely from being tracked in a separate PR IMO.
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.
I got bitten early on with git commit -a
when working with submodules that I never use it ever :)
7c00c61
to
db5a326
Compare
Co-authored-by: Brigitta Sipőcz <[email protected]>
Thanks @rossbar! |
General maintenance to remove warnings from running the notebooks, including handling deprecated functions in scipy, which just released and rc for 1.12.
As noted in #154, failing on runtime warnings is not currently baked into CI. The way that I did this locally was to convert the tutorials into pure python files:
Then run each of the python files with
-Werror
:It's not perfect, but maybe something along these lines could be added to CI to resolve #154