Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Fix streamplot set #2500

Closed
wants to merge 3 commits into from
Closed

Conversation

tonysyu
Copy link
Contributor

@tonysyu tonysyu commented Oct 6, 2013

Currently, streamplot returns a not-so-useful StreamplotSet containing the line and arrow collections drawn on the figure. One major problem was that the arrow collection created was never added to the axes (each arrow patch was added separately). Unfortunately, making the arrows a true PatchCollection is a bit difficult: FancyArrowPatch doesn't seem to play nice with PatchCollection.

To fix these issues this PR:

  • Uses FancyArrow instead of FancyArrowPatch
  • Makes StreamplotSet a CompositeCollection.

The consequences are that:

  • Tests break since the different arrow classes draw with different sizes and at different locations
  • arrowstyle needs to be deprecated as an argument to streamplot
  • CompositeCollection may be a bit too magical (it automatically delegates attribute access to sub-collections)

Alternatives:

  • Fix FancyArrowPatch to work with PatchCollection. This probably requires some transform knowledge that I'm missing.
  • Instead of CompositeCollection, just add some subset of collection methods/attrs that the set should support.

Note: Changes to pyplot.py were generated by boilerplate.py.

@WeatherGod
Copy link
Member

Hmmm, I think my vote would be to fix FancyArrowPatch. Although, I have to
say that the CompositeCollection implementation might be useful with my
LogicalCollection idea that I have been toying with.

@@ -85,13 +86,21 @@ def streamplot(axes, x, y, u, v, density=1, linewidth=None, color=None,
if linewidth is None:
linewidth = matplotlib.rcParams['lines.linewidth']

if arrowstyle is not None:
print(arrowstyle)
warn("`arrowstyle` parameter is deprecated and no longer used")
Copy link
Member

Choose a reason for hiding this comment

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

Use the matplotlib deprecation warning (I think it is in cbook)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it is less a deprecation, more an outright removal, so I'd consider raising an Exception (others - does that fit with how you would do things here?)

@tacaswell
Copy link
Member

@tonysyu Where does this stand? In any case it needs a re-base again.

@tonysyu
Copy link
Contributor Author

tonysyu commented Jan 7, 2014

@tacaswell Thanks for bumping the PR. I'm not really sure where this stands, though. I submitted this PR to get feedback about some of the design decisions, but I don't think I'm any closer to reaching a conclusion.

In retrospect, I think CompositeCollection maybe a bit too magical, and unless it is useful in other code, it's a bit over engineered.

I really don't like breaking the API and replacing FancyArrow with FancyArrowPatch, but I don't know how much work would be involved in making FancyArrowPatch behave correctly.

@pelson
Copy link
Member

pelson commented Jan 9, 2014

Closing until @tonysyu is happy with the PR.

@pelson pelson closed this Jan 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants