Skip to content

Logit scale, changes in LogitLocator and LogitFormatter #14512

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 14 commits into from
Jul 29, 2019

Conversation

jb-leger
Copy link
Contributor

@jb-leger jb-leger commented Jun 9, 2019

PR Summary

I had a lot of bugs on the labelling on logit scale. For example, the following code:

import numpy as np 
import scipy as sp 
import scipy.stats 
import matplotlib.pyplot as plt 
 
s = np.linspace(-5,5,10000) 
s2 = np.linspace(-10,10,10000) 
lims = [(1e-10,1-1e-10),(1e-5,1-1e-5),(11e-2,15e-2)] 
for k,lim in enumerate(lims): 
    fig,ax = plt.subplots() 
    ax.set_yscale('logit') 
    ax.plot(s,sp.stats.norm.cdf(s)) 
    for i in range(9): 
        ax.plot(s2,sp.stats.t(i+1).cdf(s2)) 
    ax.grid() 
    ax.set_ylim(*lim) 
    fig.savefig(f'/tmp/g{k}.png')

Actual behavior

I obtain for the very large zoom:

g0

For normal zoom:

g1

For zoomed axis:

g2

Behavior on the proposed branch

All this graphs are obtained with the same code as above.

I change the locator to allow different behavior, on large zoom, some decade are not proposed on majors, but on minors. No minor ticks used for subdivision of decades. Label are present only on major ticks. I obtain:

f0

On normal zoom, all decades are shown on major, minor are indicating subdecade. Most of the time only major ticks are labelled, but when only a few of major are present, a spaced subset of minors can be labelled. I obtain :

f1

On a very zoomed graph, the logiscale is kind-of linear, so the MaxNLocator is used (in particular, my LogitLocator inherit from MaxNLocator to allows fallback to MaxNLocator in this case, I obtain:

f2

Also, two little things:

  • for ticks labelling, the precision of format is adaptative, and depends of the diff between the labelled tick and neighbors,
  • formater have use_oveline method, which allow use the survival notation (\overline{x} mean 1-x). Not enabled by default.

Should solve #13698.

PR Checklist

I putted my comments on each item.

  • Has Pytest style unit tests
    • I tried to test the most critical behaviors of my code.
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
    • Except for some new parameters, I did not add features.
  • Documentation is sphinx and numpydoc compliant
    • I tried, but I did not test, and I don't know how to test that.
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
    • I don't know if this change is major. I think it is not.
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way
    • No backward incompatible API changes.

@jb-leger jb-leger force-pushed the logit_scale_stuff branch 2 times, most recently from cfbaeb9 to 943e025 Compare June 10, 2019 10:16
@jb-leger
Copy link
Contributor Author

I force push to add some more tests (because codedev was complaining) and I rebased on master to resolve conflicts.

@jb-leger jb-leger force-pushed the logit_scale_stuff branch 4 times, most recently from 7d5e45b to 9cb21df Compare June 10, 2019 16:08
@jb-leger
Copy link
Contributor Author

Ok, finish! I add all the test I can add. Sorry for the noise of many force-push. It was only to keep a clean history.

@jb-leger
Copy link
Contributor Author

I push some change, if I have a good understanding of @anntzer comments in #14545.

  • I add the minor property in LogitLocator to keep the API compat.
  • all my new attributes was already private.
  • I switch at most as possible argument in keyword-only.

@anntzer
Copy link
Contributor

anntzer commented Jun 14, 2019

Thanks :)
From 30,000 feet the PR looks nice, but it's also quite a mouthful so it may take a while to properly review, just letting you know.

@jb-leger jb-leger force-pushed the logit_scale_stuff branch 2 times, most recently from 02c4caf to 5bdca67 Compare June 17, 2019 21:13
@jb-leger
Copy link
Contributor Author

Thanks @anntzer for you review. I take all you comments, I mark them as resolved (sorry for that if I didn't have to do this).

For readability I did'nt rebase for squashing commits, I can squash subsets of commits if needed.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Three comments to look at all of them are very minor.

No protest to this going in as-is (hence approving) but would like @jb-leger to comment before I click the green button.

@@ -2079,13 +2254,13 @@ def decade_up(x, base=10):
return base ** lx


def is_decade(x, base=10):
def is_decade(x, base=10, *, rtol=1e-10):
Copy link
Member

Choose a reason for hiding this comment

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

Should this be atol instead of rtol ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a absolute tolerance after log transform, but a relative tolerance before log transform, therefore, for me both are correct, so I follow @anntzer choice.

"""
Return a short formatted string representation of a number.
"""
if value < 0.1:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment about why these threshold were picked? It seems odd that we get the 1-N format only from 0.9 < v < 1

Copy link
Contributor Author

@jb-leger jb-leger Jul 21, 2019

Choose a reason for hiding this comment

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

It is only because I dislike format as 1.31e-1 or 1-2.31e-1. More precisely thresholds choosen to have a exponent less or equal than -2.

Comment added.

"""Return the locations of the ticks."""
vmin, vmax = self.axis.get_view_interval()
return self.tick_values(vmin, vmax)
@property
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to keep to compatibility with the old LogitLocator which has a minor attribute.

@tacaswell
Copy link
Member

Thanks for your work on this @jb-leger !

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Some documentation corrections. This may look like a lot of comments, but it's really just the same two comments: 1. capitalize docstrings, and 2. use our style of marking defaults. I added a bunch of suggestions on all of them so you should be able to just accept (most of) them easily.

@jb-leger
Copy link
Contributor Author

Thanks @tacaswell for your review. Sorry for the delay, I was away any kind of computers.

@jb-leger
Copy link
Contributor Author

Thanks @QuLogic for all your comments and suggestions. It was the first time I use the functionnality of github for applying suggestions as commits.

@jb-leger jb-leger force-pushed the logit_scale_stuff branch from 73b7904 to d81550e Compare July 22, 2019 20:54
@jb-leger
Copy link
Contributor Author

Rebased on master, to fix the errors implied by changes on CI (for example package rename, fixed by this commit).

I hope tests will pass now.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 22, 2019
@jb-leger jb-leger force-pushed the logit_scale_stuff branch from 2335616 to e113aff Compare July 23, 2019 09:25
@jb-leger
Copy link
Contributor Author

jb-leger commented Jul 23, 2019

Rebased to master for resolve conflict introduced by merge #14654.

@tacaswell tacaswell merged commit b2783ed into matplotlib:master Jul 29, 2019
@jb-leger jb-leger deleted the logit_scale_stuff branch July 29, 2019 20:11
@jb-leger jb-leger mentioned this pull request Jul 30, 2019
6 tasks
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.

4 participants