Drop Python 3.9 and make Ruff happy #341

jl-wynen · 2024-05-17T08:58:48Z

It may be easiest to review by going through the issues separately. Apply pyupgrade fixes is the lest important here.

nvaytet · 2024-05-17T12:42:55Z

It may be easiest to review by going through the issues separately. Apply pyupgrade fixes is the lest important here.

Did you mean going through the commits separately? ;-)
Also, is the pyupgrade fixes the most or the least important?

nvaytet · 2024-05-17T12:53:23Z

docs/gallery/interactive-masking.ipynb

@@ -320,7 +332,8 @@
   "mimetype": "text/x-python",
   "name": "python",
   "nbconvert_exporter": "python",
-   "pygments_lexer": "ipython3"
+   "pygments_lexer": "ipython3",
+   "version": "3.10.14"


I wonder if we can get ruff to also strip this version number out?

You could open an issue to ask them to add this feature.

nvaytet · 2024-05-17T12:59:41Z

docs/gallery/interactive-masking.ipynb

+    "r._tool.click(250, 170)"
+   ]
+  },
+  {


Why do we need a new cell here? Is is because of the import? Can we put the import at the top of the cell then? I think it would be best to have a few hidden cells as possible.

Yes, because of the import. I did not see that the cells are hidden. Merged them again.

docs/gallery/rectangle-selection.ipynb

src/plopp/backends/matplotlib/canvas.py

nvaytet · 2024-05-17T13:18:36Z

src/plopp/backends/matplotlib/canvas.py

@@ -19,15 +19,13 @@ def _to_floats(x: np.ndarray) -> np.ndarray:
    return mdates.date2num(x) if np.issubdtype(x.dtype, np.datetime64) else x


-def _none_if_not_finite(x: Union[float, int, None]) -> Union[float, int, None]:
+def _none_if_not_finite(x: float | None) -> float | int | None:


Why was int removed here (and other places)?

Because int | float is the same as float to mypy. This is technically not clean because int is not a subclass of float. But mypy treats it as such. And ruff pointed that out.

But mypy treats it as such

I don't completely understand why we want to make mypy happy when it's asking for something which is not technically correct. Also, it seems weird that the int was removed from the input type, but not from the return type?

I don't completely understand why we want to make mypy happy when it's asking for something which is not technically correct.

We have to make one of the type checkers happy, otherwise we aren't really testing out the type annotations. The reasoning is given in this PEP https://peps.python.org/pep-0484/#the-numeric-tower. We either open issues upstream or live with the fact float includes int with python type hints 🤷

Ok for the tower, but it still doesn't answer why the return type still has float | int?

I don't know. I can't think of a case where it would make a difference.
Technically, PEP484 only specifies this behaviour for arguments, not return types. But I tried it out with mypy and it seems to behave the same for the return type.

nvaytet · 2024-05-17T13:22:11Z

.copier-answers.yml

 _src_path: https://github.com/scipp/copier_template.git
 description: Visualization library for Scipp
 max_python: '3.12'
-min_python: '3.9'
+min_python: '3.10'


I think you also need to bump the python version for the scipp wheel in the nightly.txt requirement.

jl-wynen · 2024-05-17T13:25:30Z

Did you mean going through the commits separately? ;-)

Yes, that might make it easier. But you already looked at it...

Also, is the pyupgrade fixes the most or the least important?

Least. It contains largely automated updates of the type hint syntax.

jl-wynen · 2024-05-17T13:54:54Z

I don't understand why the docs build fails. It seems like it wants to access StatigFig.toolbar but that does not exist. This broke in the dependency-update commit. @nvaytet do you have any idea what might be going on?

nvaytet · 2024-05-21T06:42:51Z

It seems like it wants to access StatigFig.toolbar but that does not exist.

This would mean it's trying to make a static figure when it should be making an interactive one...
To make interactive figs in the docs, we set a variable in the conf.py:

nbsphinx_execute_arguments = [
    "--Session.metadata=scipp_sphinx_build=True",
]

Could it be a change in sphinx that broke this?

Alternatively a change in matplotlib or ipympl?

jl-wynen · 2024-05-21T12:49:23Z

The problem only occurs when both matplotlib and ipympl are updated.

It seems like is_sphinx_build does not even get called. So either self.is_widget() (backends/matplotlib/canvas.py:162) now returns False or to_widget is not even called. I'm having a hard time tracking down the cause.

But I'm confused about what you said. Should it actually make an interactive figure? The condition is self.is_widget() and not is_sphinx_build(). So that should always be False for docs builds. Why would it have made an interactive figure before?

nvaytet · 2024-05-21T13:12:22Z

According to the ipympl docs, you can either use %matplotlib widget or %matplotlib ipympl to activate the backend (the latter seems to be a new thing).
If I use %matplotlib ipympl, the docs build seems to pass.

So apprently the old command no longer works? Does this mean we have to go and change all our notebooks with interactive plots? :-(

jl-wynen · 2024-05-21T15:15:21Z

Allegedly, there is no difference between the two when running in Jupyter: matplotlib/ipympl#529
Is this a bug upstream?

…ave changed in latest version of matplotlib/ipympl

nvaytet · 2024-05-21T20:19:41Z

It's weird, when you are in a notebook, and you ask for the backend with matplotlib.get_backend(), you get 'module://ipympl.backend_nbagg' with both %matplotlib widget and %matplotlib ipympl.
However, when building sphinx docs, if you print matplotlib.get_backend() it returns just 'widget' with %matplotlib widget in the notebook (I am guessing it will print 'ipympl' with %matplotlib ipympl but I have not actually checked that).

In any case, I pushed a fix which seems to work.

For context, we were determining if we should make a static or interactive fig based on whether the string ipympl was found in the backend.

jl-wynen · 2024-05-22T07:08:08Z

Thanks, Neil! I was not aware of this piece of code.

jl-wynen · 2024-05-22T07:08:30Z

Are you happy with the rest?

jl-wynen added 5 commits May 17, 2024 09:59

Copier update

20f692f

Make ruff happy

e9b2af0

Drop Python 3.9

766dea2

Apply pyupgrade fixes

695be66

Make ruff happy again w py310

72ef157

jl-wynen requested a review from nvaytet May 17, 2024 09:25

nvaytet reviewed May 17, 2024

View reviewed changes

jl-wynen added 2 commits May 17, 2024 15:29

Merge hidden cells

a4fe3fb

Update dependencies

8d5060d

check for both ipympl and widget in backend string as this seems to h…

3feae9b

…ave changed in latest version of matplotlib/ipympl

nvaytet approved these changes May 22, 2024

View reviewed changes

jl-wynen merged commit 9f8066b into main May 22, 2024
3 checks passed

jl-wynen deleted the drop-py39 branch May 22, 2024 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Python 3.9 and make Ruff happy #341

Drop Python 3.9 and make Ruff happy #341

jl-wynen commented May 17, 2024

nvaytet commented May 17, 2024

nvaytet May 17, 2024

jl-wynen May 17, 2024

nvaytet May 17, 2024

jl-wynen May 17, 2024

nvaytet May 17, 2024

jl-wynen May 17, 2024

nvaytet May 21, 2024

MridulS May 21, 2024

nvaytet May 21, 2024

jl-wynen May 21, 2024

nvaytet May 17, 2024

jl-wynen commented May 17, 2024

jl-wynen commented May 17, 2024 •

edited

Loading

nvaytet commented May 21, 2024

jl-wynen commented May 21, 2024

nvaytet commented May 21, 2024

jl-wynen commented May 21, 2024

nvaytet commented May 21, 2024 •

edited

Loading

jl-wynen commented May 22, 2024

jl-wynen commented May 22, 2024

Drop Python 3.9 and make Ruff happy #341

Drop Python 3.9 and make Ruff happy #341

Conversation

jl-wynen commented May 17, 2024

nvaytet commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jl-wynen commented May 17, 2024

jl-wynen commented May 17, 2024 • edited Loading

nvaytet commented May 21, 2024

jl-wynen commented May 21, 2024

nvaytet commented May 21, 2024

jl-wynen commented May 21, 2024

nvaytet commented May 21, 2024 • edited Loading

jl-wynen commented May 22, 2024

jl-wynen commented May 22, 2024

jl-wynen commented May 17, 2024 •

edited

Loading

nvaytet commented May 21, 2024 •

edited

Loading