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

Adjust number of ticks based on length of axis #5588

Merged
merged 13 commits into from
Dec 14, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Nov 30, 2015

Follow on to #5494.

@mdboom mdboom added this to the next major release (2.0) milestone Nov 30, 2015
will be removed. If prune==None, no ticks will be removed.

"""
if 'nbins' in kwargs:
Copy link
Member

Choose a reason for hiding this comment

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

need to over-ride set_params and black-list nbins there as well.

@tacaswell
Copy link
Member

set_params should blacklist nbins as it will be ignored.

@mdboom
Copy link
Member Author

mdboom commented Dec 1, 2015

set_params should blacklist nbins as it will be ignored.

You mean, raise an exception if it's there?

@tacaswell
Copy link
Member

Yes

On Tue, Dec 1, 2015 at 6:22 PM Michael Droettboom notifications@github.com
wrote:

set_params should blacklist nbins as it will be ignored.

You mean, raise an exception if it's there?


Reply to this email directly or view it on GitHub
#5588 (comment)
.

@mdboom
Copy link
Member Author

mdboom commented Dec 1, 2015

Done.

@efiring
Copy link
Member

efiring commented Dec 2, 2015

Does this mean that the default will now always be AutoLocator? And that a user will no longer be able to use locator_params(nbins=4) to adjust it? If so, I think this is a mistake. Letting the initial selection of nbins be done by an algorithm is good; but making it impossible to override this with a kwarg is not good. Or have I misunderstood?

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

@efiring: That's a good point. I've simplified this so that nbins == None can be used to indicate auto mode. This is the default for "new style", whereas classic mode will use the old default of 9. This actually significantly reduces the patch size as well.

@tacaswell
Copy link
Member

It might be better to use the string 'auto' as the default? It is clearer
we will be doing something magic.

On Wed, Dec 2, 2015, 07:54 Michael Droettboom notifications@github.com
wrote:

@efiring https://github.com/efiring: That's a good point. I've
simplified this so that nbins == None can be used to indicate auto mode.
This is the default for "new style", whereas classic mode will use the old
default of 9. This actually significantly reduces the patch size as well.


Reply to this email directly or view it on GitHub
#5588 (comment)
.

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

It might be better to use the string 'auto' as the default? It is clearer we will be doing something magic.

Sure.

@WeatherGod
Copy link
Member

Right, "auto" is better for meaning automatic, meanwhile, None can continue
to mean "do the default thing".

On Wed, Dec 2, 2015 at 8:24 AM, Michael Droettboom <notifications@github.com

wrote:

It might be better to use the string 'auto' as the default? It is clearer
we will be doing something magic.

Sure.


Reply to this email directly or view it on GitHub
#5588 (comment)
.

@efiring
Copy link
Member

efiring commented Dec 2, 2015

Good. Now, I am wondering whether there is any point in making the additional AutoLocator class; wouldn't it be simpler to fold the small modification of __init__ into MaxNLocator itself? Then the selection of one functionality or the other is entirely a matter of how the nbins kwarg is set.

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

Good. Now, I am wondering whether there is any point in making the additional AutoLocator class; wouldn't it be simpler to fold the small modification of init into MaxNLocator itself? Then the selection of one functionality or the other is entirely a matter of how the nbins kwarg is set.

AutoLocator isn't new -- it's been there forever. It sets up reasonable defaults for MaxNLocator. I agree it doesn't have much reason to exist, but removing would theoretically break backward compatibility.

@mdboom mdboom force-pushed the dynamic-ticking branch 2 times, most recently from fed1ae3 to 0920ddc Compare December 9, 2015 20:43
@mdboom
Copy link
Member Author

mdboom commented Dec 9, 2015

Just rebased. I think this is good-to-go once Travis passes.

@mdboom
Copy link
Member Author

mdboom commented Dec 14, 2015

And... it's green (on Travis)

efiring added a commit that referenced this pull request Dec 14, 2015
Adjust number of ticks based on length of axis
@efiring efiring merged commit 7a29ad1 into matplotlib:master Dec 14, 2015
@efiring
Copy link
Member

efiring commented Dec 14, 2015

@mdboom, I suspect some corner cases will turn up and tweaks might be needed, but I think it makes sense to merge this so that it will get more exercise.

@mdboom
Copy link
Member Author

mdboom commented Dec 14, 2015

Agreed. Thanks.

@tacaswell
Copy link
Member

This was merged to master, but the merge is in the v2.0.x branch. I think something fishy went on around b7c7c5e but I am not sure.

efiring added a commit that referenced this pull request Dec 20, 2015
Adjust number of ticks based on length of axis
@tacaswell
Copy link
Member

backported as 7488da6

@efiring
Copy link
Member

efiring commented Dec 21, 2015

@mdboom, I finally took a look to see what this does on master, with a minimal plot plot([1,2,3]) on OS-X, using the qt4agg backend, and I think there is a problem: it is coming up with 11 ticks. Yes, the labels all fit, but it looks bad--very cluttered. I suggest that the default max number should be much smaller, more similar to what it used to be.

@mdboom
Copy link
Member Author

mdboom commented Dec 22, 2015

@efiring: You're suggesting a hard maximum? Just adding more space is unlikely to be a good fix, since in cases with many small axes you do want to allow the ticks to get reasonably close together.

@efiring
Copy link
Member

efiring commented Dec 22, 2015

Yes, a hard maximum is what I had in mind. Then the behavior would not change for the default single-axes-per-figure case, but the multiple axes case would still be supported.

@efiring
Copy link
Member

efiring commented Dec 22, 2015

Of course that hard maximum could be controllable with yet another rcParam if necessary. We didn't have one for that before, though.

@mdboom
Copy link
Member Author

mdboom commented Dec 22, 2015

I'm fine with just setting the max to the old hard-coded value (which appears to be 9) and we can always add an rcParam later if people want more control.

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