Skip to content

Changed the return type of streamplot to a container object #24388

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Changed the return type of streamplot to a container object #24388

wants to merge 2 commits into from

Conversation

story645
Copy link
Member

@story645 story645 commented Nov 7, 2022 •
edited
Loading

The idea of using a container object for streamplots was discussed in #803, but they settled on basically a lightweight datatype object. My reasoning for switching to a container was a combo of container providing a nicer repl (kind of) and so that there would be more uniformity around artist return types cause the docs can all point to .Container

I'm sort of concerned that no tests seem to fail given the discussion in the original PR is that containers don't play well with scalar mappables, but that could be due to changes in how container was implemented?

This came up cause of the discussion in #24274 around good examples for ArtistAnimation being the artists that return containers or contourset. Looking at contourset, that seemed very specialized and like it made more sense to think of this like the containers and maybe build out to support some of the other things mentioned in the note for this method.

The actual change is super lightweight since it's switching one named tuple for the other, which means anyone currently relying on a StreamplotSet object doesn't lose the lines or arrows attribute.

It seemed easiest to weave this in is by

  1. deprecating StreamplotSet
  2. adding StreamplotContainer

I didn't write up a whats new or api_change 'cause I know this needs to be discussed first.

Eta: @efiring what sorta information did you want this container to hold?

@story645
Changed streamplot return type: StreamplotSet->StreamplotContainer
833d45a 
Deprecated StreamplotSet
Added StreamplotContainer
@story645 story645 marked this pull request as draft November 7, 2022 04:23
@story645
Copy link
Member Author

story645 commented Nov 11, 2022 •
edited
Loading

Discussed on call and .remove doesn't work b/c there's no remove method on collections, b/c that is apparently a hard problem,

matplotlib/lib/matplotlib/artist.py

Lines 230 to 234 in 04ce3e9

# TODO: the fix for the collections relim problem is to move the
# limits calculation into the artist itself, including the property of
# whether or not the artist should affect the limits. Then there will
# be no distinction between axes.add_line, axes.add_patch, etc.
# TODO: add legend support

one that I think requires writing an iterator for collections, so uh putting this as a note for when I get back to this...

ETA: Container should possibly be a dataclass instead of inheriting from tuple if the main reason it's inheriting from tuple is the named attributes.

@story645 story645 mentioned this pull request Feb 2, 2023
Closed
@anntzer
Copy link
Contributor

anntzer commented Feb 2, 2023 •
edited
Loading

On a tangentially related point, I wonder whether we can stop using FancyArrowPatches (which are relatively complex) to draw the arrowheads in the streamplot (note that we don't even use them to draw arrow bodies...), and instead simply use markers with the appropriate rotation? (It would still have to be a PathCollection to support colormapping, but not a PatchCollection.)
Edit: Actually perhaps not, a nice thing about FancyArrowPatch it that it provides data-coordinates rotation for free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
API: changes status: needs rebase topic: collections and mappables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@story645 @anntzer