Skip to content

Implemented a new Style Cycle feature for Issue #2841 #4314

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

Closed
wants to merge 7 commits into from

Conversation

jasonliw93
Copy link
Contributor

Implemented a style cycle feature for Issue #2841
We incorporated the requested feature for a linestyle cycle that behaves like the existing color cycle, however we created a new class called Cycle to add the ability to cycle through less common line styles as well.
Currently, it is only used and owned by axes for plotting lines, but this could be extensible to other plots in the future as well.

@jasonliw93 jasonliw93 changed the title Issue2841 Implemented a new Style Cycle feature for Issue #2841 Apr 5, 2015
@efiring
Copy link
Member

efiring commented Apr 5, 2015

How does this compare to #4258?

@jasonliw93
Copy link
Contributor Author

Our implementation is simpler, we don't handle the composition of different styles. We simply cycle through the given styles and combine them when you call next(). However, a seperate function could be created to help the users create cycles similar to the one made by tacaswell.
We also integrated our some of our input validation with rcParams.

from matplotlib import rcParams


class Cycle:
Copy link
Member

Choose a reason for hiding this comment

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

This needs to inherit from object

@tacaswell
Copy link
Member

I think that #4258 is simpler and more powerful (but I am biased 😉 ).

Left some comments about technical issues, but I am not a huge fan of this design, it seems too brittle.

@WeatherGod
Copy link
Member

Should probably take a look at #4258

On Sun, Apr 5, 2015 at 7:25 PM, jasonliw93 notifications@github.com wrote:

Implemented a style cycle feature for Issue #2841
#2841
We incorporated the requested feature for a linestyle cycle that behaves
like the existing color cycle, however we created a new class called Cycle
to add the ability to cycle through less common line styles as well.
Currently, it is only used and owned by axes for plotting lines, but this

could be extensible to other plots in the future as well.

You can view, comment on, or merge this pull request online at:

#4314
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#4314.

@WeatherGod
Copy link
Member

And, I just noticed that the PR got its title changed, so the subsequent comments didn't show up in the same thread in gmail... ignore me.

@jasonliw93
Copy link
Contributor Author

Thank you for all the comments tacaswell, this was a good learning experience nonetheless and feel free to take any ideas from our implementation if this doesn't get merged.

@tacaswell
Copy link
Member

One major design design difference is that your Cycler is the iterator, where as mine is iterable. It is a subtle difference and I am not completely sure which way is the right way to go. I think that it would be very difficult to do composition using your method (as how do you compose cycles at different points along their trajectories?).

On the other hand, I do appreciate the minimal amount of API breakage that this has. On the other other hand, it may be an API that needs to be broken to move forward.

@tacaswell tacaswell added this to the proposed next point release milestone Apr 9, 2015
@tacaswell tacaswell mentioned this pull request Apr 19, 2015
@tacaswell
Copy link
Member

Closing in favor of a solution built on #4258

@jasonliw93 @lichri12 Even though this didn't get merged thank you for you work!

Closing PRs with out merging is my least favorite part of maintaining mpl 😞 .

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.

5 participants