Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

MNT: fix __array__ to numpy#22975

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedMay 4, 2022
edited
Loading

PR Summary

Second attempt:closes#22973

Arrays sometimes don't have all the methods arrays should have, so
add another check here. Plot requires both ndim and shape and this
will extract the numpy array if x does not have those attributes.
Otherwise leave the object alone, because unit support (currently only
in plot) requires the object to retain the unit info.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

pllim reacted with heart emoji
@jklymak
Copy link
MemberAuthor

This fails the unit handling...

@mhvk
Copy link
Contributor

mhvk commentedMay 4, 2022

That's tricky: so it seems that theQuantity class being tested is not anndarray, but tries to behave similarly so it can be used as an array. But it also has an__array__ method as well which converts it to a plainndarray. I guess the problem is that the presence of__array__ can mean both "I'm not an array, so if you want an array, use this" or "I'm an array duck type, but if you want a realndarray, use this". I think ideally this whole routine would be something like

return x if isinstance(x, (np.ndarray, ndarray_ducktype)) else np.asarray(x)

But the real question is how to determine whether something is anndarray_ducktype...

@jklymakjklymakforce-pushed themnt-fix-array-numpy-second-try branch 2 times, most recently from085b1f9 to6ccd2c1CompareMay 5, 2022 04:38
@@ -1333,7 +1333,9 @@ def _check_1d(x):
"""Convert scalars to 1D arrays; pass-through arrays as is."""
# Unpack in case of e.g. Pandas or xarray object
x = _unpack_to_numpy(x)
if not hasattr(x, 'shape') or len(x.shape) < 1:
if (not hasattr(x, 'shape') or
not hasattr(x, 'ndim') or
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This seems to fix the problem. and see the test. However I am not sure if this is really on us, or theKernel, which is missingndim. Or maybe we should not be relying on ndim.

Copy link
Member

Choose a reason for hiding this comment

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

Isndim alwayslen(shape)? (I guess not, but otherwise it make make sense to just use that instead?)

Copy link
Member

Choose a reason for hiding this comment

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

There are 120ndim in the Python code base (although some are in tests or non-Python-code), so even though we probably should fix it here, in the long run it may be better thatKernel getsndim.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is thatKernel does not really properly behave like anndarray, so it does need to be converted to an array here. We're (ab)using the fact that it does not definendim as a proxy for "I'm not a proper duck type, just happen to haveshape". Really one would like thenp.asduckarray that has been discussed in numpy...

@timhoffm
Copy link
Member

timhoffm commentedMay 5, 2022
edited
Loading

But the real question is how to determine whether something is anndarray_ducktype ...

That cannot be answered generally. Duck-typing is an operational concept: If it provides the functions I need, it's a duck-type to me. Duck types are never perfect (at the very least they'd failisinstance() behavior), but if I only need objects thatquack like a duck I don't care whether itwalks like a duck as well.

The only thing we could do is hard-code a list of known good-enough duck-types. Otherwise we'd have to query the object upfront for all the behavior we want which is practially impossible.
A third theoretical option would be to fully embrace ducktyping: Try with the object as is, if that fails convert the object to a numpy array and retry. However, that can lead to performance issues and more importantly our code is not designed for retries, so we might get into consistency issues if the first try partially modifies some state.

@jklymak
Copy link
MemberAuthor

Just writing as I sort this out in my head.

Most of our methods do something like

x,y=self._process_unit_info([("x",x), ("y",y)],kwargs)x=np.ma.ravel(x)# or alternately np.asarray(x)y=np.ma.ravel(y)

by which point we have set the units, and x and y are now typendarray.

Theax.plot method, on the other hand, needs to unpack x and y into individual arguments so we do:

x=_check_1d(xy[0])y=_check_1d(xy[1])self.axes.xaxis.update_units(x)self.axes.yaxis.update_units(y)

However, we do not want to lose units yet, and indeed we do not, at least according totest_units.py::test_numpy_facade, which claims we have the ability to transform units at any time withax.xaxis.set_units. However, note thatax.xaxis.set_units absolutely fails withscatter (see#9713 (comment)) so dragging around the unit information only works forplot currently.

Hence we are in a bit of a Gordian knot of compatibility. We have enshrinedpint as something we support, but only forplot apparently. We allow__array__ but currently depend on it havingndim andshape. If an array doesn't haveshape then wenp.atleast_1d it. I suppose we can do the same thing to arrays withoutndim.

@jklymakjklymak marked this pull request as ready for reviewMay 6, 2022 21:10
@jklymak
Copy link
MemberAuthor

This works, but I really feel this is all a mess. Hopefully one of our RSE's can work on this...

@jklymakjklymak added this to thev3.5.3 milestoneMay 6, 2022
@jklymakjklymak added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelMay 6, 2022
Arrays sometimes don't have all the methods arrays should have, soadd another check here.  Plot requires both ndim and shape and thiswill extract the numpy array if x does not have those attributes.Otherwise leave the object alone, because unit support (currently onlyin plot) requires the object to retain the unit info.
@jklymakjklymakforce-pushed themnt-fix-array-numpy-second-try branch frome2da0f9 to29ce2c2CompareMay 6, 2022 21:14
@mhvk
Copy link
Contributor

mhvk commentedMay 6, 2022

@jklymak - indeed, also for astropy unit support works really well for plots, but not for images (cannot give units for an extent). Would be nice if that could also be exchanged. I think for the particular bug that triggered all this, we can also adjust on our side -- it is not something used all the time.

@jklymak
Copy link
MemberAuthor

Just to clarify my comment, again while I am thinking about it - ideallyplot would do the same thing as the other methods and convert to numpy immediately after the units have been determined. But currently we cannot becauseplot (andonlyplot) tries to preserve unit information.

In my mind what theproper way to do this would be to

  1. determine units of the object and pass to the axis
  2. use the units to covert the unit-full object to numpy into some base unit for the axis (so if the converter allows 'feet', 'miles', and 'meters', convert to floats in meters).
  3. Allow theaxis to scale its data in the units that the user wants (so if they specify 'feet', add a transform from meters to feet to the displayed data, but keep the underlying data in meters).

This way Matplotlib wouldn't need to drag around the extra units internally, it would just have a transform on each axis.

@mhvk
Copy link
Contributor

mhvk commentedMay 6, 2022

yes, that makes sense

@dopplershift
Copy link
Contributor

I think the reason it's currently set up this way with Pint andplot() is becauseplot() is the only one I've personally needed to adjust axis units--whereas most of my use of other plot units is spatially, where units only matter for the z/image axis (e.g.imshow,contour).

I'm not sure how you can tweak the current conversion interface without breaking backwards compatibility...

@jklymak
Copy link
MemberAuthor

I think my proposal would keep the same public user interface but change the internal representation of data passed in w units.

I think it could also be done so that most features work for downstream libraries the way they do now for scatter (not able to change axis units properly) but to have the plot-like full feature downstream libraries would need to add more logic to their locators and formatters.

@jklymak
Copy link
MemberAuthor

See#23015 for more discussion of the convert-immediately strategy for dealing with units.

@jklymak
Copy link
MemberAuthor

Despite the discussion above, I think this should go in as-is for 3.5.3....

Copy link
Member

@oscargusoscargus left a comment

Choose a reason for hiding this comment

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

Should indeed go in for 3.5.3 to fix the regression.

@timhoffmtimhoffm merged commitda84e38 intomatplotlib:mainMay 18, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestMay 18, 2022
@jklymakjklymak deleted the mnt-fix-array-numpy-second-try branchMay 18, 2022 12:12
QuLogic added a commit that referenced this pull requestMay 19, 2022
…975-on-v3.5.xBackport PR#22975 on branch v3.5.x (MNT: fix __array__ to numpy)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@mhvkmhvkmhvk left review comments

@timhoffmtimhoffmtimhoffm approved these changes

@oscargusoscargusoscargus approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: units and array ducktypes
Projects
None yet
Milestone
v3.5.3
Development

Successfully merging this pull request may close these issues.

[Bug]: v3.5.2 causing plot to crash when plotting object with__array__ method
6 participants
@jklymak@mhvk@timhoffm@dopplershift@tacaswell@oscargus

[8]ページ先頭

©2009-2025 Movatter.jp