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

matplotlib 3.8.0 breaks a visualization example for the docs. #15319

Closed
1 task
WilliamJamieson opened this issue Sep 15, 2023 · 10 comments · Fixed by #15328
Closed
1 task

matplotlib 3.8.0 breaks a visualization example for the docs. #15319

WilliamJamieson opened this issue Sep 15, 2023 · 10 comments · Fixed by #15328

Comments

@WilliamJamieson
Copy link
Contributor

WilliamJamieson commented Sep 15, 2023


https://readthedocs.org/projects/astropy/builds/21934027/ as an example.

These are due to a failure in either:

Having either of those lines enabled produces the same traceback as what is listed in the build log linked above.

This seams like a real failure, and is not being caught by the current test suite.

@pllim
Copy link
Member

pllim commented Sep 15, 2023

Example Gallery is a maintenance liability, maybe we should revisit #7242 with a different solution since the grand plan from Jonathan Sick never happened.

@pllim pllim added the 🔥 Critical label Sep 15, 2023
@ksunden
Copy link

ksunden commented Sep 15, 2023

This is quite puzzling... it bisects to matplotlib/matplotlib#26335, which is some rather innocuous looking optimization changes, in particular it is the addition of these lines in colors.py::Normalize.autoscale_None

        if isinstance(A, np.ma.MaskedArray):
            # we need to make the distinction between an array, False, np.bool_(False)
            if A.mask is False or not A.mask.shape:
                A = A.data

Which should just be stripping the mask off prior to calling min/max as a performance consideration...

And autoscale_None isn't even in the call stack for where the exception happens‽

I will state that there is a behavior difference between a masked_Quantity and a normal Quantity array:

>>> f = np.linspace(1,10,11) * astropy.units.m
>>> f
<Quantity [ 1. ,  1.9,  2.8,  3.7,  4.6,  5.5,  6.4,  7.3,  8.2,  9.1,
           10. ] m>
>>> f.min()
<Quantity 1. m>
>>> f.max()
<Quantity 10. m>
>>> np.ma.MaskedArray(f)
masked_Quantity(data=[ 1. ,  1.9,  2.8,  3.7,  4.6,  5.5,  6.4,  7.3,
                       8.2,  9.1, 10. ],
                mask=False,
          fill_value=1e+20)
>>> ma = np.ma.MaskedArray(f)
>>> ma.min()
masked_Quantity(data=1.,
                mask=False,
          fill_value=1e+20)
>>> ma.max()
masked_Quantity(data=10.,
                mask=False,
          fill_value=1e+20)
>>> ma.data
<Quantity [ 1. ,  1.9,  2.8,  3.7,  4.6,  5.5,  6.4,  7.3,  8.2,  9.1,
           10. ] m>
>>> ma.data.min()
<Quantity 1. m>
>>> ma.data.max()
<Quantity 10. m>
>>> float(ma.min())
1.0
>>> float(ma.data.min())
UnitConversionError: 'Unit("m")' is not a scaled version of 'Unit(dimensionless)'
UnitConversionError: 'm' (length) and '' (dimensionless) are not convertible
TypeError: only dimensionless scalar quantities can be converted to Python scalars
Full traceback from float(ma.data.min())
>>> ma.min().units
╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ in <module>:1                                                                                    │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: 'MaskedArray' object has no attribute 'units'
>>> float(ma.min())
1.0
>>> float(ma.data.min())
╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ /home/kyle/src/scipy/astropy/astropy/units/quantity.py:981 in to_value                           │
│                                                                                                  │
│    978 │   │   │   # with "==", but that calculates the scale that we need anyway.               │979 │   │   │   # TODO: would be better for `unit.to` to have an in-place flag.               │980 │   │   │   try:                                                                          │
│ ❱  981 │   │   │   │   scale = self.unit._to(unit)                                               │
│    982 │   │   │   except Exception:                                                             │
│    983 │   │   │   │   # Short-cut failed; try default (maybe equivalencies help).               │984 │   │   │   │   value = self._to_value(unit, equivalencies)                               │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/core.py:1159 in _to                                   │
│                                                                                                  │
│   1156 │   │   │   ):                                                                            │
│   1157 │   │   │   │   return self_decomposed.scale / other_decomposed.scale                     │
│   1158 │   │                                                                                     │
│ ❱ 1159 │   │   raise UnitConversionError(f"'{self!r}' is not a scaled version of '{other!r}'")   │
│   1160 │                                                                                         │
│   1161def to(self, other, value=UNITY, equivalencies=[]):                                   │
│   1162 │   │   """                                                                               │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
UnitConversionError: 'Unit("m")' is not a scaled version of 'Unit(dimensionless)'

During handling of the above exception, another exception occurred:

╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ /home/kyle/src/scipy/astropy/astropy/units/quantity.py:1349 in __float__                         │
│                                                                                                  │
│   1346 │   # Numerical types                                                                     │
│   1347 │   def __float__(self):                                                                  │
│   1348 │   │   try:                                                                              │
│ ❱ 1349 │   │   │   return float(self.to_value(dimensionless_unscaled))                           │
│   1350 │   │   except (UnitsError, TypeError):                                                   │
│   1351 │   │   │   raise TypeError(                                                              │
│   1352 │   │   │   │   "only dimensionless scalar quantities can be "                            │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/quantity.py:984 in to_value                           │
│                                                                                                  │
│    981 │   │   │   │   scale = self.unit._to(unit)                                               │
│    982 │   │   │   except Exception:                                                             │
│    983 │   │   │   │   # Short-cut failed; try default (maybe equivalencies help).               │
│ ❱  984 │   │   │   │   value = self._to_value(unit, equivalencies)                               │
│    985 │   │   │   else:                                                                         │
│    986 │   │   │   │   value = self.view(np.ndarray)                                             │
│    987 │   │   │   │   if not is_effectively_unity(scale):                                       │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/quantity.py:890 in _to_value                          │
│                                                                                                  │
│    887 │   │   │   equivalencies = self._equivalencies                                           │
│    888 │   │   if not self.dtype.names or isinstance(self.unit, StructuredUnit):                 │
│    889 │   │   │   # Standard path, let unit to do work.                                         │
│ ❱  890 │   │   │   return self.unit.to(                                                          │
│    891 │   │   │   │   unit, self.view(np.ndarray), equivalencies=equivalencies                  │
│    892 │   │   │   )                                                                             │
│    893                                                                                           │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/core.py:1195 in to                                    │
│                                                                                                  │
│   1192 │   │   if other is self and value is UNITY:                                              │
│   1193 │   │   │   return UNITY                                                                  │
│   1194 │   │   else:                                                                             │
│ ❱ 1195 │   │   │   return self._get_converter(Unit(other), equivalencies)(value)                 │
│   1196 │                                                                                         │
│   1197 │   def in_units(self, other, value=1.0, equivalencies=[]):                               │
│   1198 │   │   """                                                                               │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/core.py:1124 in _get_converter                        │
│                                                                                                  │
│   1121 │   │   │   │   │   │   else:                                                             │
│   1122 │   │   │   │   │   │   │   return lambda v: b(converter(v))                              │
│   1123 │   │   │                                                                                 │
│ ❱ 1124 │   │   │   raise exc                                                                     │
│   1125 │                                                                                         │
│   1126def _to(self, other):                                                                 │
│   1127 │   │   """                                                                               │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/core.py:1107 in _get_converter                        │
│                                                                                                  │
│   1104 │   │                                                                                     │
│   1105 │   │   # if that doesn't work, maybe we can do it with equivalencies?                    │
│   1106 │   │   try:                                                                              │
│ ❱ 1107 │   │   │   return self._apply_equivalencies(                                             │
│   1108 │   │   │   │   self, other, self._normalize_equivalencies(equivalencies)                 │
│   1109 │   │   │   )                                                                             │
│   1110 │   │   except UnitsError as exc:                                                         │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/core.py:1085 in _apply_equivalencies                  │
│                                                                                                  │
│   1082 │   │   unit_str = get_err_str(unit)                                                      │
│   1083 │   │   other_str = get_err_str(other)                                                    │
│   1084 │   │                                                                                     │
│ ❱ 1085 │   │   raise UnitConversionError(f"{unit_str} and {other_str} are not convertible")      │
│   1086 │                                                                                         │
│   1087 │   def _get_converter(self, other, equivalencies=[]):                                    │
│   1088 │   │   """Get a converter for values in ``self`` to ``other``.                           │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
UnitConversionError: 'm' (length) and '' (dimensionless) are not convertible

During handling of the above exception, another exception occurred:

╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ in <module>:1                                                                                    │
│                                                                                                  │
│ /home/kyle/src/scipy/astropy/astropy/units/quantity.py:1351 in __float__                         │
│                                                                                                  │
│   1348 │   │   try:                                                                              │
│   1349 │   │   │   return float(self.to_value(dimensionless_unscaled))                           │
│   1350 │   │   except (UnitsError, TypeError):                                                   │
│ ❱ 1351 │   │   │   raise TypeError(                                                              │
│   1352 │   │   │   │   "only dimensionless scalar quantities can be "                            │
│   1353 │   │   │   │   "converted to Python scalars"                                             │
│   1354 │   │   │   )                                                                             │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
TypeError: only dimensionless scalar quantities can be converted to Python scalars
>>>
So now the question becomes "What should happen here?" because I fear we are/have been losing the unit info for vmin/vmax when we should not have been previously. and now we have it, but try to simply pass through float()

@pllim
Copy link
Member

pllim commented Sep 15, 2023

Thanks for the detailed analysis, @ksunden ! We do have this that supposedly bridge the gap between units and matplotlib, but looks like it is not working here.

def quantity_support(format="latex_inline"):

Pinging visualization maintainers (@larrybradley @astrofrog @Cadair) and units maintainers (@mhvk @nstarman).

@ksunden
Copy link

ksunden commented Sep 15, 2023

The thing that I realized after posting that is that this is in coloring, which matplotlib actually doesn't do unit things for currently (though arguably should... but that is a longer term story)

Normalize objects have a fair bit in common with Scale objects, in mpl-land, but they are actually different things.

@ksunden
Copy link

ksunden commented Sep 15, 2023

So now the question I have is:

"Is setting the color to a unitful value actually valid?"

if you pass c=m33altazs_July12_to_13.az.base to the scatter line, the example again works

we had just been inadvertently working around protections against casting to raw floats by having a masked array in between.

I think stripping units for current implementation of coloring is required, the question is should that be more explicit and be forced to be on the input (which would mean no change to mpl) or should mpl always drop units (at which point, perhaps the fix is to go from asanyarray to asarray in that method, which risks some unnecessary copies, but I think is more generic and ensures we get something that can be passed through float

Or we can just revert the optimization as having unintended consequences, but that seems relatively fragile, since the fact that we wrap with a masked_array seems like an implementation detail that shouldn't be locked in...

@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

I think conceptually one would want c=xxx to take c' = (c-c.min())/c.ptp() and then be allowed to presume that the result behaves as a regular float array between 0 and 1. Passing c through np.ma.MaskedArray would seem bad form, since that class does not behave well with ndarray subclasses.

That said, more practically, if it was a coincidence this worked, and all we're dealing with here is an example, I think it is fine to change our example to just write c=...az.value and be done with this!

@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

As this is holding up everything on astropy, I went with a try at the quick-fix on our side; see #15328.

EDIT: Fixed reference PR number.

@mhvk
Copy link
Contributor

mhvk commented Sep 16, 2023

#15328 (adding .value) does seem to do the trick. I'd say we go with it for now, to get things unstuck...

EDIT: Fixed reference PR number.

@pllim
Copy link
Member

pllim commented Sep 16, 2023

So... is explicitly passing in .value a good enough compromise? In other words, does #15328 close this out?

@pllim
Copy link
Member

pllim commented Sep 16, 2023

(Feel free to re-open or open new issues if there are unresolved matters we need to follow-up on. Thanks!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants