NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

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

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

Jump to bottom

Merged

QuLogic merged 6 commits intomatplotlib:mainfromericpre:add_EllipseCollection_setter

Apr 1, 2024

Merged

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

QuLogic merged 6 commits intomatplotlib:mainfromericpre:add_EllipseCollection_setter

Apr 1, 2024

Conversation

Copy link

Member

PR summary

Currently, the following example:

importmatplotlib.pyplotaspltfrommatplotlib.collectionsimportEllipseCollectionimportnumpyasnprng=np.random.default_rng(0)widths= (2, )heights= (3, )angles= (45, )offsets=rng.random((10,2))*10fig,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))*2new_heights=rng.random((10,2))*3new_angles=rng.random((10,2))*180ec.set(widths=new_widths,heights=new_heights,angles=new_angles)

will fails with the errors:

File~\Dev\matplotlib\lib\matplotlib\artist.py:147in<lambda>cls.set=lambdaself,**kwargs:Artist.set(self,**kwargs)File~\Dev\matplotlib\lib\matplotlib\artist.py:1221insetreturnself._internal_update(cbook.normalize_kwargs(kwargs,self))File~\Dev\matplotlib\lib\matplotlib\artist.py:1213in_internal_updatereturnself._update_props(File~\Dev\matplotlib\lib\matplotlib\artist.py:1187in_update_propsraiseAttributeError(AttributeError:EllipseCollection.set()gotanunexpectedkeywordargument'widths'

PR checklist

[n/a] "closes #0000" is in the body of the PR description tolink the related issue
new and changed code istested
[n/a]Plotting related features are demonstrated in anexample
New Features andAPI Changes are noted with adirective and release note
Documentation complies withgeneral anddocstring guidelines

ericpre force-pushed theadd_EllipseCollection_setter branch fromc553db5 to013f69bCompare

July 22, 2023 11:41

oscargus reviewed

Jul 23, 2023

View reviewed changes

Copy link

Member

oscargus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

(Are there getters as well?)

lib/matplotlib/tests/test_collections.pyShow resolvedHide resolved

lib/matplotlib/collections.pyShow resolvedHide resolved

Copy link

MemberAuthor

ericpre commentedJul 24, 2023

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!

melissawm added New feature topic: collections and mappables labels

Jul 24, 2023

ericpre mentioned this pull request

Jul 29, 2023

AddU,V andC setter toQuiver#26410

Open

3 tasks

oscargus approved these changes

Aug 4, 2023

View reviewed changes

Copy link

Member

oscargus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

oscargus added this to thev3.8.0 milestone

Aug 4, 2023

This was referencedAug 4, 2023

Add rectangle collection with rotation#26457

Open

Marker Collectionhyperspy/hyperspy#3148

Merged

ksunden modified the milestones:v3.8.0,v3.9.0

Aug 8, 2023

Copy link

Member

tacaswell commentedJan 25, 2024

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

Addwidths,heights andangles setter toEllipseCollection

5a9cd4e

ericpre force-pushed theadd_EllipseCollection_setter branch frombdbc78e tobdde2e9Compare

January 28, 2024 09:48

Copy link

MemberAuthor

ericpre commentedJan 28, 2024

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.

ericpre added the status: needs review label

Feb 2, 2024

timhoffm requested changes

Mar 8, 2024

View reviewed changes

Copy link

Member

timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

QuLogic commentedMar 21, 2024

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

Copy link

MemberAuthor

ericpre commentedMar 21, 2024

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

@timhoffm, does it sound good to overwrite theset 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.

Copy link

Member

timhoffm commentedMar 21, 2024•
edited
Loading

@timhoffm, does it sound good to overwrite theset method?

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

matplotlib/lib/matplotlib/artist.py

Lines 139 to 150 in9e18a34

	ifnothasattr(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=lambdaself,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 explicitset 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 toset_XYUVC(), but that would be something likeset_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 requested a review fromtimhoffm

March 23, 2024 10:38

Copy link

MemberAuthor

ericpre commentedMar 23, 2024

I added a_check_length method to check the length of the input parameter usingoffsets as reference for the number of items in the collection.

ericpre force-pushed theadd_EllipseCollection_setter branch from0cfe854 tobdde2e9Compare

March 26, 2024 22:07

Copy link

MemberAuthor

ericpre commentedMar 26, 2024

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

QuLogic reviewed

Mar 28, 2024

View reviewed changes

doc/users/next_whats_new/add_EllipseCollection_setters.rst OutdatedShow resolvedHide resolved

lib/matplotlib/collections.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_collections.py

Comment on lines +431 to +433

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

Copy link

Member

QuLogicMar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

ericpreMar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link

Member

timhoffmMar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

MemberAuthor

ericpreMar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

SimplifyEllipseCollection setter and tidy up typing

729467c

ericpre force-pushed theadd_EllipseCollection_setter branch frombdde2e9 to729467cCompare

March 28, 2024 14:51

ericpre commented

Mar 28, 2024

View reviewed changes

Copy link

MemberAuthor

ericpre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks@QuLogic, I have addressed your comments!

lib/matplotlib/collections.py OutdatedShow resolvedHide resolved

lib/matplotlib/tests/test_collections.py

Comment on lines +431 to +433

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

Copy link

MemberAuthor

ericpreMar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?