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

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

Open
ericpre wants to merge7 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromericpre:quiver_setters

Conversation

@ericpre
Copy link
Member

PR summary

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

Example:

importmatplotlib.pyplotaspltimportmatplotlib.quiverasmquiverimportnumpyasnpfig,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

@ericpreericpre changed the titleAddU,V andC toQuiverAddU,V andC setter toQuiverJul 29, 2023
@ericpreericpre mentioned this pull requestAug 7, 2023
21 tasks
Copy link
Member

@rcomerrcomer 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 anelse branch of theif U is None: loop above.

Copy link
MemberAuthor

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

@rcomerrcomerSep 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 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.

@ericpreericpreforce-pushed thequiver_setters branch 4 times, most recently fromf3436d2 to37c1ff8CompareSeptember 5, 2023 20:43
@ericpre
Copy link
MemberAuthor

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

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

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.

Copy link
MemberAuthor

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

@rcomerrcomer 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 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 aset_XY method rather than offsets?
  • I tried commenting out the lastset_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 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.

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

@rcomerrcomerSep 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 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.


@property
defXY(self):
returnnp.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) 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?

@rcomer
Copy link
Member

I just noticed there is overlap here with#22407.

self.set_array(C)
self.stale=True

defset_offsets(self,xy):
Copy link
Member

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.

Copy link
MemberAuthor

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! 😅

Copy link
MemberAuthor

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).

Copy link
MemberAuthor

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
  • theX,Y setter/getter are matching the constructor arguments
  • it is an alias to setter/getter of the base class (Collection).

@tacaswell
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 aset_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).

@rcomerrcomer removed the status: needs comment/discussionneeds consensus on next step labelJan 25, 2024
@rcomer
Copy link
Member

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

@ianhi
Copy link
Contributor

Please lift whatever code is helpful from that PR!

rcomer reacted with thumbs up emoji

@ericpre
Copy link
MemberAuthor

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

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

@timhoffm
Copy link
Member

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

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

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

@QuLogic
Copy link
Member

Similar ping here as in#26375

@ericpre
Copy link
MemberAuthor

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

@timhoffmtimhoffm 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 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.

@ericpre
Copy link
MemberAuthor

Similarly as in the#26375, the original motivation of this PR was to be able to use theQuiver.set method (for exampleqc.set(X=U, Y=V, C=C)) which requires to add theset_* 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 theset_* method is more consistent with the behaviour of artist/collection.

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

@timhoffm
Copy link
Member

timhoffm commentedMar 25, 2024•
edited
Loading

Thanks for the clarifcation.

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

The motivation forset_XYUVC was simultaneously to allow shape validation and letting people change the shape. Changing the will not work throughset(X=X, Y=Y, U=U, V=V, C=C) with simple delegation through the individualset_X methods. As in#26375 (comment), you would have to reimplementset 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 settersset_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.

@ericpreericpreforce-pushed thequiver_setters branch 2 times, most recently from994e782 tob5d3c16CompareMarch 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

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?


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

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.

Copy link
Member

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)

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

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)

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

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)

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

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
ifCisnotNoneorself._CisnotNone:
C=ma.masked_invalid(
self._CifCisNoneelseC,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.

This seems extra complicated with theorand ternary:

Suggested change
ifCisnotNoneorself._CisnotNone:
C=ma.masked_invalid(
self._CifCisNoneelseC,copy=True
).ravel()
C=ma.masked_invalid(C,copy=True).ravel()ifCisnotNoneelseself._C

Copy link
Member

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?

self._Umask=mask
ifCisnotNone:
self.set_array(C)
self._N=N
Copy link
Member

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.

ifCisnotNone:
self.set_array(C)
self._N=N
self._new_UV=True
Copy link
Member

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.

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


deftest_quiver_offsets():
Copy link
Member

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?

@timhoffmtimhoffm modified the milestones:v3.9.0,v3.10.0Apr 2, 2024
@ericpre
Copy link
MemberAuthor

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

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

Reviewers

@tacaswelltacaswelltacaswell left review comments

@QuLogicQuLogicQuLogic left review comments

@ksundenksundenksunden left review comments

@timhoffmtimhoffmtimhoffm requested changes

@oscargusoscargusoscargus left review comments

@rcomerrcomerrcomer 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.

9 participants

@ericpre@rcomer@tacaswell@ianhi@timhoffm@QuLogic@ksunden@oscargus@story645
[8]ページ先頭
©2009-2025 Movatter.jp