Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
AddU,V andC setter toQuiver#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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this 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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/quiver.py Outdated
| if U is None: | ||
| U = self.U | ||
| if V is None: | ||
| 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() |
There was a problem hiding this comment.
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 anelse branch of theif U is None: loop above.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was thinking thatself.U would previously have been set through this method so should not have any invalid points to mask. Admittedly, sinceU is a public attribute itcould have been set directly by the user, but that would presumably be undesirable. See my more general comments on these public attributes.
f3436d2 to37c1ff8CompareThank you@rcomer for the review, this should all done - the failure with the doc seems to be unrelated to this PR. |
lib/matplotlib/quiver.py Outdated
| Parameters | ||
| ---------- | ||
| U : ArrayLike | None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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", likearray-like or None.
There was a problem hiding this comment.
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 usearray-like (I see one instance, for which I am to blame..., where we have anArrayLike that snuck in, but otherwise all docstrings arearray-like
Actually, numpy usesarray_like in docstrings, but I would value self consistency over anything else.
There was a problem hiding this comment.
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!
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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 newset_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_XYmethod rather than offsets? - I tried commenting out the last
set_UVCcall in the "what's new" example. I expected it might error at draw, but actually I got this
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 likeset_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 andC all need some sort of check/processing when they are set.N is justX.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 withget_ andset_ methods where the user is likely to need access.
Ping@timhoffm as there are lots of API questions here.
lib/matplotlib/quiver.py Outdated
| if U is None: | ||
| U = self.U | ||
| if V is None: | ||
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was thinking thatself.U would previously have been set through this method so should not have any invalid points to mask. Admittedly, sinceU is a public attribute itcould have been set directly by the user, but that would presumably be undesirable. See my more general comments on these public attributes.
lib/matplotlib/quiver.py Outdated
| @property | ||
| def XY(self): | ||
| return np.column_stack((self.X, self.Y)) |
There was a problem hiding this comment.
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) implementget_N andget_XY for consistency with other parts of the library. Also do we need to go through the deprecation pathway to prevent setting of these?
I just noticed there is overlap here with#22407. |
lib/matplotlib/quiver.py Outdated
| @@ -569,6 +628,19 @@ def set_UVC(self, U, V, C=None): | |||
| self.set_array(C) | |||
| self.stale = True | |||
| def set_offsets(self, xy): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
change toset_XY to match input naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point. I addedset_offsets to be consistent with other collections. An alternative would be to renameXY tooffsets everywhere? Maybe for another PR! 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have keep bothset_XY andset_offsets, withset_XY being an alias ofset_offsets and documented as such.
I still think that it would be simple to keep onlyset_offsets because of its consistency with others collections but also because it is possible to useset_X andset_Y which match the constructor (set_XY doesn't match anything really).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I removedset_XY/get_XY to keep the API simple for the following reason:
- it doesn't match the constructor arguments
- the
X,Ysetter/getter are matching the constructor arguments - it is an alias to setter/getter of the base class (
Collection).
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 |
Please lift whatever code is helpful from that PR! |
To summarize (and make sure that I understand correctly):
|
Well, they just call |
Similar ping here as in#26375 |
…roperties to avoid inconsistent state of `quiver`
3374bd9 to20d7089Compare
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 theX, Y, U, V, C attributes.
While in your original post, you correctly attested that we don't haveset_U() and hence one cannotqc.set_U(U/5), it has been possible toqc.U /= 5. Breaking such usage feels unnecessary. Even the setterqc.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 wantset_U() etc?
The minimal change here would be to addset_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 proposedset_XYUVC(), and to implementset_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 toalso prevent shape changes through the existingU ... attributes. But then, the back-compatible solution (at least for changing only data not shape) is turning them into properties and also delegating toset_XYUVC; i.e. not deprecating read and write attribute behavior.
Similarly as in the#26375, the original motivation of this PR was to be able to use the To keep things simple, I am wondering if we should also privatise |
timhoffm commentedMar 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the clarifcation.
The motivation for Since 3.9 RC is due this week, I propose a two step approach:
|
994e782 tob5d3c16Compare| 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. |
There was a problem hiding this comment.
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 haveset_X andset_Y?
| self._dpi_at_last_init = self.axes.figure.dpi | ||
| @property | ||
| def N(self): | ||
| _api.warn_deprecated("3.9", alternative="get_X().size") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All deprecations are missing thename argument. Also I thought@timhoffm suggested to not deprecate these? I don't know if that's been resolved.
There was a problem hiding this comment.
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 theset_U() etc. methods directly forwarding to the attributeself.U. None of the internal refactorings orset_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.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| """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.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| """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.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| """Returns the horizontal direction components.""" | |
| """Return the horizontal direction components.""" |
| 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() |
There was a problem hiding this comment.
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 theorand ternary:
| ifCisnotNoneorself._CisnotNone: | |
| C=ma.masked_invalid( | |
| self._CifCisNoneelseC,copy=True | |
| ).ravel() | |
| C=ma.masked_invalid(C,copy=True).ravel()ifCisnotNoneelseself._C |
There was a problem hiding this comment.
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, asC is not unmasked later. But actually, I'm a bit confused, as nothing appears to setself._C to anything?
Uh oh!
There was an error while loading.Please reload this page.
| if C is not None: | ||
| self.set_array(C) | ||
| self._N = N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is unused now, asN usesget_X.
| if C is not None: | ||
| self.set_array(C) | ||
| self._N = N | ||
| self._new_UV = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't thinkself._new_UV is used either.
| @@ -357,6 +358,131 @@ def test_collection_log_datalim(fig_test, fig_ref): | |||
| ax_ref.plot(x, y, marker="o", ls="") | |||
| def test_quiver_offsets(): | |||
There was a problem hiding this comment.
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 intest_quiver.py?
I agree that it is better to leave it to 3.10 and restart this work through incremental PRs. |
PR summary
Similarly as#26375, add setters to be able to update arrows collection using quiver collection interface.
Example:
PR checklist