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

Further support for matplotlib 3.6 #34693

Closed
kiwifb opened this issue Oct 24, 2022 · 41 comments
Closed

Further support for matplotlib 3.6 #34693

kiwifb opened this issue Oct 24, 2022 · 41 comments

Comments

@kiwifb
Copy link
Member

kiwifb commented Oct 24, 2022

#34668 was focusing on making doctests pass but did not test docbuilding.

The following plot in src/sage/plot/plot.py

    .. PLOT::

        g = plot(2*x+1,(x,0,5),ticks=[[0,1,e,pi,sqrt(20)],2],tick_formatter="latex")
        sphinx_plot(g)

is causing the building of the documentation to fail horribly.

[plotting ]  from /home/portage/sci-mathematics/sage-doc-9999/work/sage-doc-9999/src/doc/en/reference/plotting/sage/plot/plot.rst:
[plotting ] Traceback (most recent call last):
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1378, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **{**kwargs, "for_layout_only": True})
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1251, in get_tightbbox
[plotting ]     ticks_to_draw = self._update_ticks()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1198, in _update_ticks
[plotting ]     minor_locs = self.get_minorticklocs()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1431, in get_minorticklocs
[plotting ]     mask = np.isclose(tr_minor_locs[:, None], tr_major_locs[None, :],
[plotting ]   File "<__array_function__ internals>", line 180, in isclose
[plotting ]   File "/usr/lib/python3.10/site-packages/numpy/core/numeric.py", line 2373, in isclose
[plotting ]     yfin = isfinite(y)
[plotting ] TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
[plotting ] During handling of the above exception, another exception occurred:
[plotting ] Traceback (most recent call last):
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1378, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **{**kwargs, "for_layout_only": True})
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axes/_base.py", line 4428, in get_tightbbox
[plotting ]     ba = martist._get_tightbbox_for_layout_only(axis, renderer)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1380, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **kwargs)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1251, in get_tightbbox
[plotting ]     ticks_to_draw = self._update_ticks()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1198, in _update_ticks
[plotting ]     minor_locs = self.get_minorticklocs()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1431, in get_minorticklocs
[plotting ]     mask = np.isclose(tr_minor_locs[:, None], tr_major_locs[None, :],
[plotting ]   File "<__array_function__ internals>", line 180, in isclose
[plotting ]   File "/usr/lib/python3.10/site-packages/numpy/core/numeric.py", line 2373, in isclose
[plotting ]     yfin = isfinite(y)
[plotting ] TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
[plotting ] During handling of the above exception, another exception occurred:
[plotting ] Traceback (most recent call last):
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1378, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **{**kwargs, "for_layout_only": True})
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1251, in get_tightbbox
[plotting ]     ticks_to_draw = self._update_ticks()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1198, in _update_ticks
[plotting ]     minor_locs = self.get_minorticklocs()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1431, in get_minorticklocs
[plotting ]     mask = np.isclose(tr_minor_locs[:, None], tr_major_locs[None, :],
[plotting ]   File "<__array_function__ internals>", line 180, in isclose
[plotting ]   File "/usr/lib/python3.10/site-packages/numpy/core/numeric.py", line 2373, in isclose
[plotting ]     yfin = isfinite(y)
[plotting ] TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''
[plotting ] During handling of the above exception, another exception occurred:
[plotting ] Traceback (most recent call last):
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/sphinxext/plot_directive.py", line 515, in _run_code
[plotting ]     exec(code, ns)
[plotting ]   File "<string>", line 2, in <module>
[plotting ]   File "<string>", line 37, in sphinx_plot
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/figure.py", line 3448, in tight_layout
[plotting ]     engine.execute(self)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/layout_engine.py", line 180, in execute
[plotting ]     kwargs = get_tight_layout_figure(
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/_tight_layout.py", line 305, in get_tight_layout_figure
[plotting ]     kwargs = _auto_adjust_subplotpars(fig, renderer,
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/_tight_layout.py", line 82, in _auto_adjust_subplotpars
[plotting ]     bb += [martist._get_tightbbox_for_layout_only(ax, renderer)]
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1380, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **kwargs)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axes/_base.py", line 4428, in get_tightbbox
[plotting ]     ba = martist._get_tightbbox_for_layout_only(axis, renderer)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/artist.py", line 1380, in _get_tightbbox_for_layout_only
[plotting ]     return obj.get_tightbbox(*args, **kwargs)
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1251, in get_tightbbox
[plotting ]     ticks_to_draw = self._update_ticks()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1198, in _update_ticks
[plotting ]     minor_locs = self.get_minorticklocs()
[plotting ]   File "/usr/lib/python3.10/site-packages/matplotlib/axis.py", line 1431, in get_minorticklocs
[plotting ]     mask = np.isclose(tr_minor_locs[:, None], tr_major_locs[None, :],
[plotting ]   File "<__array_function__ internals>", line 180, in isclose
[plotting ]   File "/usr/lib/python3.10/site-packages/numpy/core/numeric.py", line 2373, in isclose
[plotting ]     yfin = isfinite(y)
[plotting ] TypeError: ufunc 'isfinite' not supported for the input types, and the inputs could not be safely coerced to any supported types according to the casting rule ''safe''

Tests of this plot were disabled in #34668 and now we need to remove the plot. The whole example needs to be replaced by something that works.

We could not overcome the problem of symbolic values that numpy rejects, but instead made a fix to circumvent numpy and revived the doctest and the example.

CC: @tornaria

Component: documentation

Author: Kwankyu Lee

Branch/Commit: 6458968

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/34693

@kiwifb kiwifb added this to the sage-9.8 milestone Oct 24, 2022
@kiwifb
Copy link
Member Author

kiwifb commented Oct 24, 2022

comment:1

Currently making sure that this it the only change required and providing support to sage-on-gentoo users already using matplotlib 3.6.x before pushing a branch here.

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

Author: François Bissey

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

Branch: u/tornaria/matplotlib-3.6

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

comment:2

The attached branch will let you build the doc with matplotlib 3.6.


New commits:

5501e0dSupport matplotlib 3.6

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

Commit: 5501e0d

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

comment:3

Not sure how I got to attach the wrong branch. Thankfully, I noticed.


New commits:

8b0c147Merge branch 'mpl3.6_doctest' into mpl3.6_docbuilding
4645a0dRemoving faulty plot

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

Changed commit from 5501e0d to 4645a0d

@kiwifb
Copy link
Member Author

kiwifb commented Oct 25, 2022

Changed branch from u/tornaria/matplotlib-3.6 to u/fbissey/MPL3.6_docbuilding

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 27, 2022

comment:4

Is this ready for review?

@kiwifb
Copy link
Member Author

kiwifb commented Nov 27, 2022

comment:5

Yes, it is ready.Our consensus at #34668 is that the point of this particular example is unclear. So, removing it does not seem to be a big deal.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 27, 2022

comment:6

I'm hitting the same error (ufunc 'isfinite' not supported) also in some of my own plotting code.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 27, 2022

comment:7

Possibly related: #29210 scipy.optimize.newton does not accept Sage input anymore

@kiwifb
Copy link
Member Author

kiwifb commented Nov 27, 2022

comment:8

Yes, I am now wondering if it is an issue that matplotlib (and scipy) may need to adapt to new stuff in numpy. But we cannot leave the plot in, since it breaks building the doc.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 28, 2022

comment:9

Replying to François Bissey:

Yes, it is ready. Our consensus at #34668 is that the point of this particular example is unclear.

It is clear. We can use symbolic values as tick marks. It is a useful feature. I may use it (though I never did :)

So, removing it does not seem to be a big deal.

Removing a feature to upgrade a package is a big deal.

I investigated the real cause of the failure. Fortunately I found it. It is a kind of bug in matplotlib 3.6. I reported it to upstream:

matplotlib/matplotlib#24557

Let's see how they react before we proceed.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:10

In the upstream discussion matplotlib/matplotlib#24557 (comment), they suggested to make our symbolic objects behave more float-like:

Can you add isfinite support to Expression (or a ConstExpression which may or may not already exist)? It seems to me that being able to ask if an expression is finite is something you should be able to ask.

or numpy-array-like:

Is it possible for sagemath to provide a .__array__ method to convert to numpy arrays?

I will investigate the former possibility. We have

sage: a = sqrt(2)
sage: a.is_infinity()
False

but I don't know how to "add isfinite support" to satisfy numpy...

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:11
sage: np.isfinite(2)
True

So Sage integers are numbers from the perspective of numpy. How does it do it?

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:12

BTW,

sage: np.can_cast(type(2), float, 'safe')
False

Strange...

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 29, 2022

comment:13

I don't know if that's the mechanism why it works, but our Integers are registered with the numbers ABC - https://docs.python.org/3/library/numbers.html

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:14

Replying to Matthias Köppe:

I don't know if that's the mechanism why it works, but our Integers are registered with the numbers ABC - https://docs.python.org/3/library/numbers.html

I tried that, but doesn't work. Not sure I missed something...

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:15

Voilà, I found it. It is

https://numpy.org/doc/stable/reference/arrays.interface.html

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:16

and https://numpy.org/doc/stable/reference/arrays.dtypes.html

Then

class Symbolic(object):
    __array_interface__ = {'typestr': 'f'} 
    
    def __init__(self, a, b):
        self.num = a
        self.den = b
        
    def __str__(self):
        return '{}/{}'.format(self.num, self.den)
        
    def __float__(self):
        return float(self.num / self.den)
    
import matplotlib.pyplot as plt
import numpy as np
import sympy as sym
from matplotlib.ticker import FixedLocator, NullLocator, FuncFormatter

# make data
x = np.linspace(0, 3, 100)
y = 2 * x + 1

# plot
fig, ax = plt.subplots()

ax.plot(x, y, linewidth=2.0)

a = Symbolic(1,2)

ax.xaxis.set_major_locator(FixedLocator([a, 1, 2, 3]))
ax.xaxis.set_minor_locator(NullLocator())
ax.xaxis.set_major_formatter(FuncFormatter(lambda x, pos: str(x)))

plt.show()

works. Moreover

>>> a = Symbolic(2,3)
>>> np.isfinite(a)
True
>>> np.array(a)
array(0.6666667, dtype=float32)

Hence so we now know how to make Expression float-like (from the perspective of numpy).

To implement this mechanism for Expression may require major restructuring, since the scope of Expression is much more broad beyond constant expression. As suggested in the upstream discussion, we may need ConstantExpression that would hold only constant values, and make that float-like or complex-like. I feel this would be a grand work out of my capacity though. Symbolic expression experts may do it. But the first question is if this work is worth to do. If so, this would need a separate ticket.

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 29, 2022

comment:17

Does __array_interface__ have to be a class constant, or could it be a lazy_attribute?

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:18

It can be lazy_attributes or property.

But I realized a major problem. By the mechanism, Symbolic is simply converted to floats, and you see 0.5 in the plot instead of 1/2. So this defeats the original purpose! Now it seems to me that we cannot retain the feature (supporting symbolic ticks)...

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:19

Replying to Kwankyu Lee:

By the mechanism, Symbolic is simply converted to floats, and you see 0.5 in the plot instead of 1/2. So this defeats the original purpose! Now it seems to me that we cannot retain the feature (supporting symbolic ticks)...

The rescue is to use matplotlib's FixedFormatter to provide strings for the tick values separately. This would amount to reimplementing sage's tick_formatter="latex".

Anyway, implementing this together with ConstantExpression seems a daunting task apart from the "worth to do" question.

I don't know how frequently the symbolic tick mark feature is used by sage users. If no one uses it :) then perhaps we may turn off this feature while we upgrade matplotlib to 3.6.

@kiwifb
Copy link
Member Author

kiwifb commented Nov 29, 2022

comment:20

OK. So, it is currently broken and will take some time and efforts to put it right, if there is a will.

In the meantime, the branch removes just a plot showcasing the capability because it breaks building the documentation in sage-on-distros which already have matplotlib 3.6.x available.
Should we comment the plotting code and add reference to a ticket with work in progress or just remove it until things are ready?

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 29, 2022

comment:21

How about just commenting them out mentioning this ticket?

But no one uses it? I really don't know. How about posting on sage-devel to prevent some one later to complain that his/her code doesn't work anymore...

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 29, 2022

comment:22

I definitely use this feature (comment:6); -1 on removing the test

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 30, 2022

comment:23

Replying to Matthias Köppe:

I definitely use this feature (comment:6); -1 on removing the test

+1.

Meanwhile I learned enough matplotlib to make up a simple fix for this ticket.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 30, 2022

Changed author from François Bissey to Kwankyu Lee

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 30, 2022

Changed branch from u/fbissey/MPL3.6_docbuilding to u/klee/MPL3.6_docbuilding

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 30, 2022

New commits:

f0a7703Add back doctest

@kwankyu

This comment has been minimized.

@kwankyu
Copy link
Collaborator

kwankyu commented Nov 30, 2022

Changed commit from 4645a0d to f0a7703

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Changed commit from f0a7703 to a8741d8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

8371dd7Missing part of the fix
dbfb953Merge branch 'develop' into t/34693/MPL3.6_docbuilding
a8741d8Edit the doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Changed commit from a8741d8 to 6458968

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

6458968More edis of the doctest

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 1, 2022

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 1, 2022

comment:28

I can confirm that this also fixes the issue in my plotting code. Thanks a lot for working on it!

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 1, 2022

comment:29

Thanks. I also checked the branch with #34796 that upgrades matplotlib to 3.6. It works well.

@vbraun
Copy link
Member

vbraun commented Dec 11, 2022

Changed branch from u/klee/MPL3.6_docbuilding to 6458968

@vbraun vbraun closed this as completed in 949866d Dec 11, 2022
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants