Add `U`, `V` and `C` setter to `Quiver` #26410

ericpre · 2023-07-29T09:36:13Z

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

[n/a] "closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
[n/a] Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

rcomer

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

doc/users/next_whats_new/add_Quiver_setters.rst

lib/matplotlib/quiver.py

rcomer · 2023-09-02T13:07:44Z

lib/matplotlib/quiver.py

+        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()


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

It is not possible because of the mask.

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 · 2023-09-06T09:38:21Z

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

oscargus · 2023-09-06T10:29:37Z

lib/matplotlib/quiver.py

+
+        Parameters
+        ----------
+        U : ArrayLike | None


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.

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.

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

doc/users/next_whats_new/add_Quiver_setters.rst

rcomer

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

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.

rcomer · 2023-09-17T12:51:25Z

lib/matplotlib/quiver.py

+        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()


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.

rcomer · 2023-09-17T12:56:15Z

lib/matplotlib/quiver.py

+
+    @property
+    def XY(self):
+        return np.column_stack((self.X, self.Y))


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?

rcomer · 2023-10-11T06:40:14Z

I just noticed there is overlap here with #22407.

tacaswell · 2024-01-25T20:41:23Z

lib/matplotlib/quiver.py

@@ -569,6 +628,19 @@ def set_UVC(self, U, V, C=None):
            self.set_array(C)
        self.stale = True

+    def set_offsets(self, xy):


change to set_XY to match input naming.

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

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

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

tacaswell · 2024-01-25T20:47:15Z

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 · 2024-01-26T09:59:52Z

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.

ianhi · 2024-01-26T16:48:27Z

Please lift whatever code is helpful from that PR!

ericpre · 2024-01-28T13:18:35Z

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?

timhoffm · 2024-03-10T21:29:14Z

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)

QuLogic · 2024-03-21T20:27:57Z

Similar ping here as in #26375

…roperties to avoid inconsistent state of `quiver`

ericpre · 2024-03-23T15:03:31Z

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.

timhoffm

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.

ericpre · 2024-03-25T08:21:18Z

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.

timhoffm · 2024-03-25T09:24:27Z

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.

…erage

QuLogic · 2024-04-02T00:33:57Z

lib/matplotlib/quiver.py

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


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

QuLogic · 2024-04-02T04:00:56Z

lib/matplotlib/quiver.py


            self._dpi_at_last_init = self.axes.figure.dpi

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


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.

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.

QuLogic · 2024-04-02T04:02:32Z

lib/matplotlib/quiver.py

+        self.set_XYUVC(X=X)
+
+    def get_X(self):
+        """Returns the positions in the horizontal direction."""


Suggested change

"""Returns the positions in the horizontal direction."""

"""Return the positions in the horizontal direction."""

QuLogic · 2024-04-02T04:02:45Z

lib/matplotlib/quiver.py

+        self.set_XYUVC(Y=Y)
+
+    def get_Y(self):
+        """Returns the positions in the vertical direction."""


Suggested change

"""Returns the positions in the vertical direction."""

"""Return the positions in the vertical direction."""

QuLogic · 2024-04-02T04:03:03Z

lib/matplotlib/quiver.py

+        self.set_XYUVC(U=U)
+
+    def get_U(self):
+        """Returns the horizontal direction components."""


Suggested change

"""Returns the horizontal direction components."""

"""Return the horizontal direction components."""

QuLogic · 2024-04-02T04:09:02Z

lib/matplotlib/quiver.py

+        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()


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

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?

lib/matplotlib/quiver.py

QuLogic · 2024-04-02T04:16:36Z

lib/matplotlib/quiver.py

        if C is not None:
            self.set_array(C)
+        self._N = N


This is unused now, as N uses get_X.

QuLogic · 2024-04-02T04:16:51Z

lib/matplotlib/quiver.py

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


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

QuLogic · 2024-04-02T04:23:20Z

lib/matplotlib/tests/test_collections.py

@@ -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():


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

ericpre · 2024-04-02T08:01:23Z

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

ericpre force-pushed the quiver_setters branch from b089cfa to d69a520 Compare July 29, 2023 09:54

ericpre changed the title ~~Add U, V and C to Quiver~~ Add U, V and C setter to Quiver Jul 29, 2023

ericpre force-pushed the quiver_setters branch from d69a520 to 704ae87 Compare July 29, 2023 11:20

story645 added the topic: plotting methods label Jul 31, 2023

rcomer added the New feature label Aug 5, 2023

ericpre mentioned this pull request Aug 7, 2023

Marker Collection hyperspy/hyperspy#3148

Merged

21 tasks

rcomer reviewed Sep 2, 2023

View reviewed changes

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

oscargus reviewed Sep 6, 2023

View reviewed changes

doc/users/next_whats_new/add_Quiver_setters.rst Outdated Show resolved Hide resolved

ericpre force-pushed the quiver_setters branch from 37c1ff8 to 527510b Compare September 6, 2023 10:37

ksunden mentioned this pull request Sep 6, 2023

DOC: consistency in docstrings of formatting of array-like #26709

Merged

5 tasks

ericpre force-pushed the quiver_setters branch from 527510b to ef4615e Compare September 7, 2023 07:20

rcomer reviewed Sep 17, 2023

View reviewed changes

rcomer added the status: needs comment/discussion needs consensus on next step label Sep 20, 2023

github-actions bot added the status: needs rebase label Sep 27, 2023

tacaswell reviewed Jan 25, 2024

View reviewed changes

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

timhoffm mentioned this pull request Mar 8, 2024

Add widths, heights and angles setter to EllipseCollection #26375

Merged

3 tasks

timhoffm added the status: needs revision label Mar 10, 2024

tacaswell added this to the v3.9.0 milestone Mar 13, 2024

QuLogic mentioned this pull request Mar 13, 2024

Add set_XY and set_data to Quiver #22407

Closed

6 tasks

ericpre added 3 commits March 23, 2024 09:37

Add set_U, set_V and set_C method to matplotlib.quiver.Quiver

b268585

Add set_offsets to quiver and make the attribute (N and XY) p…

10adf0b

…roperties to avoid inconsistent state of `quiver`

Add release note

92848b2

ericpre force-pushed the quiver_setters branch from ef4615e to 1bbfcef Compare March 23, 2024 11:53

github-actions bot removed the status: needs rebase label Mar 23, 2024

ericpre force-pushed the quiver_setters branch 2 times, most recently from 3374bd9 to 20d7089 Compare March 23, 2024 14:13

ericpre added 3 commits March 23, 2024 14:34

Add set_XYUVC method to be used for all quiver setter

e5b42b1

Privatise quiver.Quiver.Umask

c8d4924

Fix typing

aba8d9b

ericpre force-pushed the quiver_setters branch from 20d7089 to aba8d9b Compare March 23, 2024 14:40

timhoffm requested changes Mar 24, 2024

View reviewed changes

ericpre force-pushed the quiver_setters branch 2 times, most recently from 994e782 to b5d3c16 Compare March 27, 2024 08:32

Don't check length of arguments in quiver collection and increase cov…

9b5bc95

…erage

ericpre force-pushed the quiver_setters branch from b5d3c16 to 9b5bc95 Compare March 28, 2024 08:21

QuLogic reviewed Apr 2, 2024

View reviewed changes

timhoffm modified the milestones: v3.9.0, v3.10.0 Apr 2, 2024

github-actions bot added the status: needs rebase label Jul 24, 2024

timhoffm mentioned this pull request Sep 18, 2024

[MNT]: Data size consistency checks in _CollectionsWithSizes #28833

Open

ksunden modified the milestones: v3.10.0, v3.11.0 Oct 16, 2024

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

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

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

		@@ -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():

Add U, V and C setter to Quiver #26410

Are you sure you want to change the base?

Add U, V and C setter to Quiver #26410

Conversation

ericpre commented Jul 29, 2023

PR summary

PR checklist

rcomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

ericpre commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer left a comment

Choose a reason for hiding this comment

rcomer Sep 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Oct 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Jan 25, 2024

rcomer commented Jan 26, 2024

ianhi commented Jan 26, 2024

ericpre commented Jan 28, 2024

timhoffm commented Mar 10, 2024

QuLogic commented Mar 21, 2024

ericpre commented Mar 23, 2024

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

ericpre commented Mar 25, 2024

timhoffm commented Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericpre commented Apr 2, 2024

Add `U`, `V` and `C` setter to `Quiver` #26410

Add `U`, `V` and `C` setter to `Quiver` #26410

rcomer Sep 17, 2023 •

edited

Loading

rcomer Sep 17, 2023 •

edited

Loading

timhoffm left a comment •

edited

Loading

timhoffm commented Mar 25, 2024 •

edited

Loading