-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Conversation
b089cfa
to
d69a520
Compare
U
, V
and C
to Quiver
(削除ここまで)U
, V
and C
setter to Quiver
(追記ここまで)
d69a520
to
704ae87
Compare
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.
lib/matplotlib/quiver.py
Outdated
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 an else
branch of the if 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 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.
f3436d2
to
37c1ff8
Compare
Thank 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
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 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
.
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 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.
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!
37c1ff8
to
527510b
Compare
527510b
to
ef4615e
Compare
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 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 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
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.
lib/matplotlib/quiver.py
Outdated
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 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.
lib/matplotlib/quiver.py
Outdated
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) 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?
I just noticed there is overlap here with #22407.
lib/matplotlib/quiver.py
Outdated
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 to set_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 added set_offsets
to be consistent with other collections. An alternative would be to rename XY
to offsets
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 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).
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 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
).
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).
Please lift whatever code is helpful from that PR!
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?
- 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)
Similar ping here as in #26375
...roperties to avoid inconsistent state of `quiver`
ef4615e
to
1bbfcef
Compare
3374bd9
to
20d7089
Compare
20d7089
to
aba8d9b
Compare
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 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.
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.
Thanks for the clarifcation.
To keep things simple, I am wondering if we should also privatise
set_XYUVC
and useset
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 theset()
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.
994e782
to
b5d3c16
Compare
b5d3c16
to
9b5bc95
Compare
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 have set_X
and set_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.
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.
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 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.
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.
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.
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.
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 the or
and ternary:
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, as C
is not unmasked later. But actually, I'm a bit confused, as nothing appears to set self._C
to anything?
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, as N
uses get_X
.
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 think self._new_UV
is used either.
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 in test_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