Skip to content

BUG: fix minpos handling and other log ticker problems #7598

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

Merged
merged 4 commits into from
Dec 12, 2016

Conversation

efiring
Copy link
Member

@efiring efiring commented Dec 9, 2016

Closes #7595, #7493, #7587.
Bbox._minpos is now initialized to [np.inf, np.inf] instead of
[1e-7, 1e-7].
Old code with a colorbar in which the formatter is set to
a LogFormatter and a SymlogNorm is used now works again (although
the results are better in 2.0 if the colorbar is left to handle
the formatter automatically).
LogLocator now has its own nonsingular() method which provides
a reasonable starting point for a log axis when no data have
been plotted.

@efiring efiring added this to the 2.0 (style change major release) milestone Dec 9, 2016
@efiring
Copy link
Member Author

efiring commented Dec 9, 2016

The log scales test is easy to fix; I need a little more time with the logit scales test.

# It's probably a colorbar with
# a format kwarg setting a LogFormatter in the manner
# that worked with 1.5.x, but that doesn't work now.
self._sublabels = set((1,)) # label powers of base
Copy link
Member

Choose a reason for hiding this comment

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

With Python 2.7, can use a set literal {1}.

@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 9, 2016

result = mtransforms.nonsingular(vmin, vmax)
return result
def nonsingular(self, vmin, vmax):
Copy link
Member

Choose a reason for hiding this comment

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

Don't we already have have a nonsingular?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but 'nonsingular' means different things for different transforms--different behaviors are needed.

Copy link
Member

Choose a reason for hiding this comment

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

can this have a leading underscore? I missread and did not notice that this was a method.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could, but there are other nonsingular methods like this that don't, and they are called as locator.nonsingular(x0, x1) in autoscale_view(). So if I were to add an underscore here, I would either have to use two different calls in autoscale_view, or add an underscore to the other locator methods with that name.

Copy link
Member

Choose a reason for hiding this comment

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

I am sold on consistency of naming.

@efiring efiring changed the title BUG: fix minpos handling and other log ticker problems [MRG] BUG: fix minpos handling and other log ticker problems Dec 10, 2016
@efiring
Copy link
Member Author

efiring commented Dec 10, 2016

Reviewers: the factoring and re-arranging I did will make it harder to evaluate the changes via the diffs, but easier to see what is going on in the LogFormatters, and how they differ, if you look at the ticker.py file itself.

@QuLogic
Copy link
Member

QuLogic commented Dec 11, 2016

I'm not too familiar with the log tickers, so I can't say too much beyond whether the examples/tests and other code work. There is a small bug with the margin handling in the logit scale, as shown by the pyplots/pyplot_scales.py and scales/scales.py examples (which are basically the same, just through alternate interfaces.)

On v2.x before this PR, the bottom of the logit axes did not respect the margin;
figure_1

on this PR, the top of the logit axes does not respect the margin:
figure_1-minpos

@efiring
Copy link
Member Author

efiring commented Dec 11, 2016

Good catch. That was a silly bug. I will push a fix in a few minutes.

if vmax >= 1:
vmax = 1 - minpos
if vmin == vmax:
return 0.1 * vmin, 10 * vmin
Copy link
Member

Choose a reason for hiding this comment

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

This does not stay vmin < 1 for all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! Fix is coming.

@tacaswell
Copy link
Member

👍 this looks like it greatly simplifies (or at least reduces code duplication in) this part of the code base!

@QuLogic
Copy link
Member

QuLogic commented Dec 11, 2016

This does appear to be working on all the examples I could try.

@efiring I'm not sure you meant to amend that last commit as it doesn't appear related to the new changes.

Closes matplotlib#7595, matplotlib#7493, matplotlib#7587.
Bbox._minpos is now initialized to [np.inf, np.inf] instead of
[1e-7, 1e-7].
Old code with a colorbar in which the formatter is set to
a LogFormatter and a SymlogNorm is used now works again (although
the results are better in 2.0 if the colorbar is left to handle
the formatter automatically).
LogLocator now has its own nonsingular() method which provides
a reasonable starting point for a log axis when no data have
been plotted.
@efiring
Copy link
Member Author

efiring commented Dec 11, 2016

@QuLogic you are correct, I made a mistake. When running git rebase -i I should have rearranged the commits. I have straightened it out in another rebase with force push.

@efiring efiring changed the title [MRG] BUG: fix minpos handling and other log ticker problems [MRG+1] BUG: fix minpos handling and other log ticker problems Dec 12, 2016
@tacaswell tacaswell merged commit 4d59447 into matplotlib:v2.x Dec 12, 2016
@QuLogic QuLogic changed the title [MRG+1] BUG: fix minpos handling and other log ticker problems BUG: fix minpos handling and other log ticker problems Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants