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

Set artist colors in one fell swoop #1147

Closed
wants to merge 2 commits into from

Conversation

dmcdougall
Copy link
Member

Currently only sets axes frame, tickmarks, ticklabels and gridlines (if they are on). This addresses issue #326.

I've thought a little bit about where this should live and considered putting it in figure.py, however I decided against this because the figure's colour (using show) and the colour that's used when executing savefig are different. I wanted to give the user the option to choose this without touching it using the set_artists_color method.

Feedback welcome.

Currently only sets axes frame, tickmarks, ticklabels and gridlines (if
they are on)
@@ -939,6 +939,11 @@ def set_color_cycle(self, clist):
self._get_lines.set_color_cycle(clist)
self._get_patches_for_fill.set_color_cycle(clist)

def set_artists_color(self, color):
Copy link
Member

Choose a reason for hiding this comment

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

obviously would need a docstring.

@@ -939,6 +939,23 @@ def set_color_cycle(self, clist):
self._get_lines.set_color_cycle(clist)
self._get_patches_for_fill.set_color_cycle(clist)

def set_artists_color(self, color):
"""
Set the colour of several artists to *color* in one move. The argument
Copy link
Member

Choose a reason for hiding this comment

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

Pedantic: to be consistent with the majority of the rest of the documentation, US English spelling, please.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about the spelling.

@WeatherGod
Copy link
Member

I am not entirely convinced of this implementation. Particularly, it is hard-coded which artists get colored, but the docs hint at it being possibly configurable. The choice of artists are (somewhat) arbitrary.

However, the fact that this PR exists points to a need for smarter color management within mpl.

@dmcdougall
Copy link
Member Author

The choice of artists was chosen to correspond to what the user wanted in #326. It might be a good idea to add the ability to pass a list of artists to colour.

Do you propose this be closed (and not merged)? If so, what would be a better way to approach #326?

@WeatherGod
Copy link
Member

Perhaps "logical" collection objects? These logical collections would hold weak references to a set of artists and would apply a particular property on all the artists in the collection. set only, no gets. Then, an axes object may have a few of these already set up for the user to use (and this could be used elsewhere). Furthermore, one could have a logical collection object that contains other logical collection objects.

This generalizes fairly nicely, I think.

@efiring
Copy link
Member

efiring commented Sep 4, 2012

Good idea.

@dmcdougall
Copy link
Member Author

I like that idea. Presumably one would have to make a choice regarding what artists belong to a given collection? Or do you think that's something the user should set up?

@WeatherGod
Copy link
Member

The idea would be that such an object merely serves as a convenience and
helps to logically group together artists in any manner a user sees fit.
While matplotlib may create some logical collections for its own purposes,
any user is free to use those collections, or to create more collections of
their own to suit their own needs.

Of course, a problem that may arise is if mpl becomes too married to such a
concept and uses it extensively internally. Then we run the risk of
conflicting settings between user-defined logical collections and
internally definded collections. This idea may need to be fleshed out some
more.

@pelson
Copy link
Member

pelson commented Sep 13, 2012

This idea may need to be fleshed out some more.

In need of a MEP? There is a label for that if so (though you don't seem to be able to set labels to PRs...).

@WeatherGod
Copy link
Member

I got a couple of MEPs to write up. I will probably do that after v1.2.0
is out the door. As far as I am concerned though, this PR should probably
be closed in favor of the new design I will write up.

@dmcdougall
Copy link
Member Author

Sounds good to me.

@WeatherGod @pelson Thanks for the insightful comments.

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