Add `widths`, `heights` and `angles` setter to `EllipseCollection` #26375

ericpre · 2023-07-22T10:10:51Z

PR summary

Currently, the following example:

import matplotlib.pyplot as plt
from matplotlib.collections import EllipseCollection
import numpy as np

rng = np.random.default_rng(0)

widths = (2, )
heights = (3, )
angles = (45, )
offsets = rng.random((10, 2)) * 10

fig, ax = plt.subplots()

ec = EllipseCollection(
    widths=widths,
    heights=heights,
    angles=angles,
    offsets=offsets,
    units='x', 
    offset_transform=ax.transData,
    )

ax.add_collection(ec)
ax.set_xlim(-2, 12)
ax.set_ylim(-2, 12)

new_widths = rng.random((10, 2)) * 2
new_heights = rng.random((10, 2)) * 3
new_angles = rng.random((10, 2)) * 180

ec.set(widths=new_widths, heights=new_heights, angles=new_angles)

will fails with the errors:

  File ~\Dev\matplotlib\lib\matplotlib\artist.py:147 in <lambda>
    cls.set = lambda self, **kwargs: Artist.set(self, **kwargs)

  File ~\Dev\matplotlib\lib\matplotlib\artist.py:1221 in set
    return self._internal_update(cbook.normalize_kwargs(kwargs, self))

  File ~\Dev\matplotlib\lib\matplotlib\artist.py:1213 in _internal_update
    return self._update_props(

  File ~\Dev\matplotlib\lib\matplotlib\artist.py:1187 in _update_props
    raise AttributeError(

AttributeError: EllipseCollection.set() got an unexpected keyword argument 'widths'

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

oscargus

It probably makes sense to use the newly added methods in the constructor.

(Are there getters as well?)

lib/matplotlib/tests/test_collections.py

lib/matplotlib/collections.py

ericpre · 2023-07-24T08:28:14Z

Thank you @oscargus for the review, this should be all done.

(Are there getters as well?)

I didn't add the getters because we don't need them and I don't know how to get their value after the transform. But if someone give me some pointers, I am happy to do that in this PR - if there are setters, it would be fair to expect getters too!

oscargus

I cannot help with the getters, unfortunately. While it is indeed ideal to have both, it is clearly better to have only setters compared to nothing.

tacaswell · 2024-01-25T20:55:18Z

We talked about this on the call and are 👍🏻 on it going in but would like getters.

The consensus on the call was for the getters to undo the geometric transformations, but don't worry about the ravel (if someone is passing in higher than 1D data, we would like to know why before bending over backwards to give it back).

ericpre · 2024-01-28T12:45:46Z

I added the getters which return the same as what is given to the setters and rebased. The CI failures shouldn't be related to the changes of this PR.

timhoffm

In a similar case #26410 (comment) where multiple related arrays can be set, we advocated for a central function that can take one or multiple of the arrays and make sure the final data (mixture of existing and new arrays) has consistent array lengths.

I suggest that we follow this approach here as well - at least for the geometry parameters widths, heights, angles, offsets. I grant that we won't check other potentially sequence-like parameters (e.g. linewidths), but AFAIK they are just cycled continuously and don't have to match in lengths.

QuLogic · 2024-03-21T07:18:19Z

A friendly ping on this; I'd like to put out 3.9 beta/rc next week.

ericpre · 2024-03-21T09:31:47Z

Thanks @QuLogic, yes it would be good to finish this PR.

@timhoffm, does it sound good to overwrite the set method? In the example added in this PR, set is used these values in a single call. This has the advantage of keeping the API minimal and consistent.

timhoffm · 2024-03-21T14:29:40Z

@timhoffm, does it sound good to overwrite the set method?

That may work, but it's a bit tricky because the set methods are auto-generated to include all properties of the class:

matplotlib/lib/matplotlib/artist.py

Lines 139 to 150 in 9e18a34

    
           if not hasattr(cls.set, '_autogenerated_signature'): 
        
               # Don't overwrite cls.set if the subclass or one of its parents 
        
               # has defined a set method set itself. 
        
               # If there was no explicit definition, cls.set is inherited from 
        
               # the hierarchy of auto-generated set methods, which hold the 
        
               # flag _autogenerated_signature. 
        
               return 
        
           cls.set = lambda self, **kwargs: Artist.set(self, **kwargs) 
        
           cls.set.__name__ = "set" 
        
           cls.set.__qualname__ = f"{cls.__qualname__}.set" 
        
           cls._update_set_signature_and_docstring()

You may have to care for _autogenerated_signature and/or _update_set_signature_and_docstring() to get explicit set signature and docstring without manually having to specify all parameters. This is some you would have to fiddle into.

Edit: I see this is slightly more complicated than #26410 (comment). I thought we make an equivalent to set_XYUVC(), but that would be something like set_widths_heights_angles_offsets() which is quite bulky, and I don't believe this would genralize well to arbitrary collections.
Thus, I'm not sure we get a viable solution with length consistency checks quickly. Therefore, if people want the fundamental functionality to set these parameters (without saftety checks) in 3.9, I would be ok to merge the current proposal as is. It's not adding any API that we'd regret later.

ericpre · 2024-03-23T10:41:29Z

I added a _check_length method to check the length of the input parameter using offsets as reference for the number of items in the collection.

ericpre · 2024-03-26T22:12:47Z

Similarly as in #26410, I remove checking the length of the parameters to leave for another PR.

doc/users/next_whats_new/add_EllipseCollection_setters.rst

lib/matplotlib/collections.py

QuLogic · 2024-03-28T09:58:23Z

lib/matplotlib/tests/test_collections.py

+    assert_array_almost_equal(ec._widths, np.array(widths).ravel() * 0.5)
+    assert_array_almost_equal(ec._heights, np.array(heights).ravel() * 0.5)
+    assert_array_almost_equal(ec._angles, np.deg2rad(angles).ravel())


Do we need to check internals now that we have the getters?

These check that the internal values are correct (multiplication of 0.5) and these are different from the values returned by the getter, so I would say that this is still worth checking these values?

Testing internals is not ideal, but OTOH these tests do not impose a big liability. It's unlikely that we will refactor the internals and if so, the tests can be easily adapted.

Alternatives would be an image comparison (not favored) or checking the bounding box of an EllipseCollection with a single entry.

If this is find to keep to check as it is I would prefer that option because it is straighforward and more simple than the bounding box alternative.

ericpre

Thanks @QuLogic, I have addressed your comments!

lib/matplotlib/collections.py

ericpre · 2024-03-28T14:53:47Z

lib/matplotlib/tests/test_collections.py

+    assert_array_almost_equal(ec._widths, np.array(widths).ravel() * 0.5)
+    assert_array_almost_equal(ec._heights, np.array(heights).ravel() * 0.5)
+    assert_array_almost_equal(ec._angles, np.deg2rad(angles).ravel())


These check that the internal values are correct (multiplication of 0.5) and these are different from the values returned by the getter, so I would say that this is still worth checking these values?

timhoffm

Take or leave the internal check.

QuLogic · 2024-04-01T22:12:09Z

I think we can ignore the AppVeyor failure here, as it seems to be something to do with wxPython.

ericpre force-pushed the add_EllipseCollection_setter branch from c553db5 to 013f69b Compare July 22, 2023 11:41

oscargus reviewed Jul 23, 2023

View reviewed changes

lib/matplotlib/tests/test_collections.py Show resolved Hide resolved

lib/matplotlib/collections.py Show resolved Hide resolved

melissawm added New feature topic: collections and mappables labels Jul 24, 2023

ericpre mentioned this pull request Jul 29, 2023

Add U, V and C setter to Quiver #26410

Open

3 tasks

oscargus approved these changes Aug 4, 2023

View reviewed changes

oscargus added this to the v3.8.0 milestone Aug 4, 2023

This was referenced Aug 4, 2023

Add rectangle collection with rotation #26457

Open

Marker Collection hyperspy/hyperspy#3148

Merged

ksunden modified the milestones: v3.8.0, v3.9.0 Aug 8, 2023

Add widths, heights and angles setter to EllipseCollection

5a9cd4e

ericpre force-pushed the add_EllipseCollection_setter branch from bdbc78e to bdde2e9 Compare January 28, 2024 09:48

ericpre added the status: needs review label Feb 2, 2024

timhoffm requested changes Mar 8, 2024

View reviewed changes

ericpre requested a review from timhoffm March 23, 2024 10:38

ericpre force-pushed the add_EllipseCollection_setter branch from 0cfe854 to bdde2e9 Compare March 26, 2024 22:07

QuLogic reviewed Mar 28, 2024

View reviewed changes

ericpre added 4 commits March 28, 2024 14:40

Add whats_new entry

673fb6a

Fix typing

f7be1b6

Use setters in constructor, add docstring to setters and improve tests

e4c5938

Add getters to EllipseColletion

45a6a6f

Simplify EllipseCollection setter and tidy up typing

729467c

ericpre force-pushed the add_EllipseCollection_setter branch from bdde2e9 to 729467c Compare March 28, 2024 14:51

ericpre commented Mar 28, 2024

View reviewed changes

timhoffm approved these changes Mar 29, 2024

View reviewed changes

QuLogic merged commit 4b42dbb into matplotlib:main Apr 1, 2024
2 of 4 checks passed

QuLogic removed the status: needs review label Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `widths`, `heights` and `angles` setter to `EllipseCollection` #26375

Add `widths`, `heights` and `angles` setter to `EllipseCollection` #26375

ericpre commented Jul 22, 2023

oscargus left a comment

ericpre commented Jul 24, 2023

oscargus left a comment

tacaswell commented Jan 25, 2024

ericpre commented Jan 28, 2024

timhoffm left a comment

QuLogic commented Mar 21, 2024

ericpre commented Mar 21, 2024

timhoffm commented Mar 21, 2024 •

edited

Loading

ericpre commented Mar 23, 2024

ericpre commented Mar 26, 2024

QuLogic Mar 28, 2024

ericpre Mar 28, 2024

timhoffm Mar 29, 2024

ericpre Mar 29, 2024

ericpre left a comment

ericpre Mar 28, 2024

timhoffm left a comment

QuLogic commented Apr 1, 2024

Add widths, heights and angles setter to EllipseCollection #26375

Add widths, heights and angles setter to EllipseCollection #26375

Conversation

ericpre commented Jul 22, 2023

PR summary

PR checklist

oscargus left a comment

Choose a reason for hiding this comment

ericpre commented Jul 24, 2023

oscargus left a comment

Choose a reason for hiding this comment

tacaswell commented Jan 25, 2024

ericpre commented Jan 28, 2024

timhoffm left a comment

Choose a reason for hiding this comment

QuLogic commented Mar 21, 2024

ericpre commented Mar 21, 2024

timhoffm commented Mar 21, 2024 • edited Loading

ericpre commented Mar 23, 2024

ericpre commented Mar 26, 2024

QuLogic Mar 28, 2024

Choose a reason for hiding this comment

ericpre Mar 28, 2024

Choose a reason for hiding this comment

timhoffm Mar 29, 2024

Choose a reason for hiding this comment

ericpre Mar 29, 2024

Choose a reason for hiding this comment

ericpre left a comment

Choose a reason for hiding this comment

ericpre Mar 28, 2024

Choose a reason for hiding this comment

timhoffm left a comment

Choose a reason for hiding this comment

QuLogic commented Apr 1, 2024

Add `widths`, `heights` and `angles` setter to `EllipseCollection` #26375

Add `widths`, `heights` and `angles` setter to `EllipseCollection` #26375

timhoffm commented Mar 21, 2024 •

edited

Loading