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

Add U, V and C setter to Quiver #26410

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

Open
ericpre wants to merge 7 commits into matplotlib:main
base: main
Choose a base branch
Loading
from ericpre:quiver_setters

Conversation

Copy link
Member

@ericpre ericpre commented Jul 29, 2023

PR summary

Similarly as #26375, add setters to be able to update arrows collection using quiver collection interface.

Example:

import matplotlib.pyplot as plt
import matplotlib.quiver as mquiver
import numpy as np
fig, ax = plt.subplots()
X = np.arange(-10, 10, 1)
Y = np.arange(-10, 10, 1)
U, V = np.meshgrid(X, Y)
M = np.hypot(U, V)
qc = mquiver.Quiver(
 ax, X, Y, U, V, M
 )
ax.add_collection(qc)
ax.autoscale_view()
qc.set_U(U/5)

PR checklist

@ericpre ericpre changed the title (削除) Add U, V and C to Quiver (削除ここまで) (追記) Add U, V and C setter to Quiver (追記ここまで) Jul 29, 2023
Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Hi @ericpre, this mostly looks good to me. Just a few minor comments.

V = self.V
# We need to ensure we have a copy, not a reference
# to an array that might change before draw().
U = ma.masked_invalid(U, copy=True).ravel()
Copy link
Member

Choose a reason for hiding this comment

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

If I have understood, this could be in an else branch of the if U is None: loop above.

Copy link
Member Author

@ericpre ericpre Sep 5, 2023

Choose a reason for hiding this comment

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

It is not possible because of the mask.

Copy link
Member

@rcomer rcomer Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking that self.U would previously have been set through this method so should not have any invalid points to mask. Admittedly, since U is a public attribute it could have been set directly by the user, but that would presumably be undesirable. See my more general comments on these public attributes.

@ericpre ericpre force-pushed the quiver_setters branch 4 times, most recently from f3436d2 to 37c1ff8 Compare September 5, 2023 20:43
Copy link
Member Author

ericpre commented Sep 6, 2023

Thank you @rcomer for the review, this should all done - the failure with the doc seems to be unrelated to this PR.

Parameters
----------
U : ArrayLike | None
Copy link
Member

@oscargus oscargus Sep 6, 2023

Choose a reason for hiding this comment

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

Suggested change
U : ArrayLike | None
U : ArrayLike | None

I think a typing/documentation-expert should comment here, but I think we still do write it a bit more "old school", like
array-like or None.

Copy link
Member

@ksunden ksunden Sep 6, 2023

Choose a reason for hiding this comment

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

Yes, we are pretty consistent in docstrings to use array-like (I see one instance, for which I am to blame..., where we have an ArrayLike that snuck in, but otherwise all docstrings are array-like

Actually, numpy uses array_like in docstrings, but I would value self consistency over anything else.

Copy link
Member Author

@ericpre ericpre Sep 7, 2023

Choose a reason for hiding this comment

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

It seems that I mixed up the docstring type convention with typing! It should be all done now!

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

I am rather more uncertain about the new set_offsets method than the original change.

  • If the goal is to have a way to change X and Y, would it be more intuitive for the user to have a set_XY method rather than offsets?
  • I tried commenting out the last set_UVC call in the "what's new" example. I expected it might error at draw, but actually I got this
    image
    It seems that U, V and C get wrapped so we have four arrows at each point instead of the expected one, and I'm not sure that's desirable. I wonder if it would be better to only allow changes to the array size when all five variables are set at once with something like set_XYUVC.

This PR also highlights that there are many attributes on this class that are public but probably shouldn't be: X, Y, XY, U, V and C all need some sort of check/processing when they are set. N is just X.size, so it's not obvious to me that the user needs access to that at all. I'm thinking we should privatise all of these and cover them with get_ and set_ methods where the user is likely to need access.

Ping @timhoffm as there are lots of API questions here.

V = self.V
# We need to ensure we have a copy, not a reference
# to an array that might change before draw().
U = ma.masked_invalid(U, copy=True).ravel()
Copy link
Member

@rcomer rcomer Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking that self.U would previously have been set through this method so should not have any invalid points to mask. Admittedly, since U is a public attribute it could have been set directly by the user, but that would presumably be undesirable. See my more general comments on these public attributes.


@property
def XY(self):
return np.column_stack((self.X, self.Y))
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense that these should not be ordinary public attributes because they should not be set directly by the user. However I wonder if it would be better to make them private attributes and (if we think it's needed) implement get_N and get_XY for consistency with other parts of the library. Also do we need to go through the deprecation pathway to prevent setting of these?

Copy link
Member

rcomer commented Oct 11, 2023

I just noticed there is overlap here with #22407.

self.set_array(C)
self.stale = True

def set_offsets(self, xy):
Copy link
Member

@tacaswell tacaswell Jan 25, 2024

Choose a reason for hiding this comment

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

change to set_XY to match input naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I added set_offsets to be consistent with other collections. An alternative would be to rename XY to offsets everywhere? Maybe for another PR! 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I have keep both set_XY and set_offsets, with set_XY being an alias of set_offsets and documented as such.

I still think that it would be simple to keep only set_offsets because of its consistency with others collections but also because it is possible to use set_X and set_Y which match the constructor (set_XY doesn't match anything really).

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed set_XY/get_XY to keep the API simple for the following reason:

  • it doesn't match the constructor arguments
  • the X, Y setter/getter are matching the constructor arguments
  • it is an alias to setter/getter of the base class (Collection).

Copy link
Member

Talked about this on weelky call

Given the @rcomer 's discovery that the sizes can miss-matched, we think it would be best to add a set_XYUVC(X=None, Y=None, U=None, V=None, C=None) method that does the same thing for X and Y that this PR proposes for UVC that also re-verifies all of the shapes are consistent (and then we get the "nice" thin wrappers that call the main one).

@rcomer rcomer removed the status: needs comment/discussion needs consensus on next step label Jan 25, 2024
Copy link
Member

rcomer commented Jan 26, 2024

I have not studied it in detail, but the proposed set_data in #22407 appears to do what we need for set_XYUVC. So potentially @ianhi's code could be pulled in here.

Copy link
Contributor

ianhi commented Jan 26, 2024

Please lift whatever code is helpful from that PR!
rcomer reacted with thumbs up emoji

Copy link
Member Author

ericpre commented Jan 28, 2024

To summarize (and make sure that I understand correctly):

  • add set_XYUVC to avoid issue with mismatching array shapes and is the only method which will allow changing the size of the arrays
  • privatise atttibutes XY, N with deprecation?
  • Add setters/getters for X, Y, U, V, C with array size checks?
tacaswell and timhoffm reacted with thumbs up emoji

Copy link
Member

  • Add setters/getters for X, Y, U, V, C with array size checks?

Well, they just call set_XYUVC which ensures length consistency with the existing data, e.g.

def set_U(self, U):
 self.set_XYUVC(U=U)

Copy link
Member

QuLogic commented Mar 21, 2024

Similar ping here as in #26375

Copy link
Member Author

ericpre commented Mar 23, 2024

This should be ready, I have used @ianhi's code from #22407 to parse the arguments and check the length consistency. Most attribute have been privatised.

Copy link
Member

@timhoffm timhoffm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewing this from a distance, I'm now very hesitant to deprecate all the X, Y, U, V, C attributes.

While in your original post, you correctly attested that we don't have set_U() and hence one cannot qc.set_U(U/5), it has been possible to qc.U /= 5. Breaking such usage feels unnecessary. Even the setter qc.U = <other array of the same size]> is a reasonable use case.

Could you please reiterate, what exactly we are trying to solve? I feel that got lost in the course of the discussion. Reading back the initial PR comment, does

to be able to update arrows collection using quiver collection interface

mean you want set_U() etc?

The minimal change here would be to add set_U(), set_V() etc. functions. In the course of the PR, we've argued that we want to prevent inconsistent shape changes through these setters. But doing the checks in the individual functions would prevent a global shape change, because you'd have inconsistent transient change while changing the individual parameters using their setters. Thus we've proposed set_XYUVC(), and to implement set_U etc. by delegating there to prevent code duplication. This is still fully back-compatible.

If I understand the motivation correctly, this would be the minimal reasonable change. One can argue that we want to also prevent shape changes through the existing U ... attributes. But then, the back-compatible solution (at least for changing only data not shape) is turning them into properties and also delegating to set_XYUVC; i.e. not deprecating read and write attribute behavior.

Copy link
Member Author

ericpre commented Mar 25, 2024

Similarly as in the #26375, the original motivation of this PR was to be able to use the Quiver.set method (for example qc.set(X=U, Y=V, C=C)) which requires to add the set_* methods - this doesn't come clearly in the first post of the PR, sorry! :)
Generally, I think that some of the collection class have grown organically and their public API have minor inconsistency and using the set_* method is more consistent with the behaviour of artist/collection.

To keep things simple, I am wondering if we should also privatise set_XYUVC and use set instead? This would avoid adding a method to the API.

Copy link
Member

timhoffm commented Mar 25, 2024 •
edited
Loading

Thanks for the clarifcation.

To keep things simple, I am wondering if we should also privatise set_XYUVC and use set instead? This would avoid adding a method to the API.

The motivation for set_XYUVC was simultaneously to allow shape validation and letting people change the shape. Changing the will not work through set(X=X, Y=Y, U=U, V=V, C=C) with simple delegation through the individual set_X methods. As in #26375 (comment), you would have to reimplement set to make that work, which is somewhat tricky.

Since 3.9 RC is due this week, I propose a two step approach:

  • For now, simply add the plain setters set_U etc. without any validation to support the set() API in 3.9. This is not worse than now, because people can aleady access the attributes. As before, if they do inconsistent changes, everything will blow up at draw time.
    Best do a new PR for that.
  • The consistency checks are more tricky than initially anticipated. Defer all consistency check discussions, i.e. this PR to 3.10.

@ericpre ericpre force-pushed the quiver_setters branch 2 times, most recently from 994e782 to b5d3c16 Compare March 27, 2024 08:32
in animations.
The API methods are set_XYUVC(), set_X(), set_Y(), set_U() and set_V(),
which can be used to change the size, orientation, and color of the
arrows; their locations are fixed when the class is instantiated.
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

This latter part of the sentence is wrong now that you have set_X and set_Y?


@property
def N(self):
_api.warn_deprecated("3.9", alternative="get_X().size")
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

All deprecations are missing the name argument. Also I thought @timhoffm suggested to not deprecate these? I don't know if that's been resolved.

Copy link
Member

@timhoffm timhoffm Apr 2, 2024

Choose a reason for hiding this comment

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

Yes, overall the state of the PR is not in the simplified form suggested in #26410 (comment). As written there, it would best be done in a separate PR and exclusively add the set_U() etc. methods directly forwarding to the attribute self.U. None of the internal refactorings or set_XYUVC() is needed for that. Alternatively, we can skip that and do everything in this PR targeting 3.10.

Either way I‘m remilestoning this PR to 3.10. If you still want the simple setter in 3.9, please open a separate PR for that.

self.set_XYUVC(X=X)

def get_X(self):
"""Returns the positions in the horizontal direction."""
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

Suggested change
"""Returns the positions in the horizontal direction."""
"""Return the positions in the horizontal direction."""

self.set_XYUVC(Y=Y)

def get_Y(self):
"""Returns the positions in the vertical direction."""
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

Suggested change
"""Returns the positions in the vertical direction."""
"""Return the positions in the vertical direction."""

self.set_XYUVC(U=U)

def get_U(self):
"""Returns the horizontal direction components."""
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

Suggested change
"""Returns the horizontal direction components."""
"""Return the horizontal direction components."""

Comment on lines +726 to +729
if C is not None or self._C is not None:
C = ma.masked_invalid(
self._C if C is None else C, copy=True
).ravel()
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

This seems extra complicated with the or and ternary:

Suggested change
if C is not None or self._C is not None:
C = ma.masked_invalid(
self._C if C is None else C, copy=True
).ravel()
C = ma.masked_invalid(C, copy=True).ravel() if C is not None else self._C

Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

But this should be okay to simplify, as C is not unmasked later. But actually, I'm a bit confused, as nothing appears to set self._C to anything?

self._Umask = mask
if C is not None:
self.set_array(C)
self._N = N
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

This is unused now, as N uses get_X.

if C is not None:
self.set_array(C)
self._N = N
self._new_UV = True
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

I don't think self._new_UV is used either.

ax_ref.plot(x, y, marker="o", ls="")


def test_quiver_offsets():
Copy link
Member

@QuLogic QuLogic Apr 2, 2024

Choose a reason for hiding this comment

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

Seems like these tests could maybe go in test_quiver.py?

@timhoffm timhoffm modified the milestones: v3.9.0, v3.10.0 Apr 2, 2024
Copy link
Member Author

ericpre commented Apr 2, 2024

I agree that it is better to leave it to 3.10 and restart this work through incremental PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@QuLogic QuLogic QuLogic left review comments

@ksunden ksunden ksunden left review comments

@timhoffm timhoffm timhoffm requested changes

@oscargus oscargus oscargus left review comments

@rcomer rcomer rcomer left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
v3.12.0
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /