Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tacaswell merged 14 commits intomatplotlib:masterfromjb-leger:logit_scale_stuff
Jul 29, 2019

Conversation

jb-leger
Copy link
Contributor

@jb-legerjb-leger commentedJun 9, 2019
edited
Loading

PR Summary

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

importnumpyasnpimportscipyasspimportscipy.statsimportmatplotlib.pyplotasplts=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)]fork,liminenumerate(lims):fig,ax=plt.subplots()ax.set_yscale('logit')ax.plot(s,sp.stats.norm.cdf(s))foriinrange(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 theMaxNLocator 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 haveuse_oveline method, which allow use the survival notation (\overline{x} mean1-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 isFlake 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 ismajor. 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-legerjb-legerforce-pushed thelogit_scale_stuff branch 2 times, most recently fromcfbaeb9 to943e025CompareJune 10, 2019 10:16
@jb-leger
Copy link
ContributorAuthor

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

@jb-legerjb-legerforce-pushed thelogit_scale_stuff branch 4 times, most recently from7d5e45b to9cb21dfCompareJune 10, 2019 16:08
@jb-leger
Copy link
ContributorAuthor

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
ContributorAuthor

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

  • I add theminor property inLogitLocator 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

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-legerjb-legerforce-pushed thelogit_scale_stuff branch 2 times, most recently from02c4caf to5bdca67CompareJune 17, 2019 21:13
@jb-leger
Copy link
ContributorAuthor

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

@tacaswelltacaswell 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 beatol instead ofrtol ?

Copy link
ContributorAuthor

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 the1-N format only from0.9 < v < 1

Copy link
ContributorAuthor

@jb-legerjb-legerJul 21, 2019
edited
Loading

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 as1.31e-1 or1-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
ContributorAuthor

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 oldLogitLocator which has aminor attribute.

@tacaswell
Copy link
Member

Thanks for your work on this@jb-leger !

Copy link
Member

@QuLogicQuLogic left a comment
edited
Loading

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
ContributorAuthor

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

@jb-leger
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

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

I hope tests will pass now.

@tacaswelltacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelJul 22, 2019
jb-legerand others added14 commitsJuly 23, 2019 11:21
Co-Authored-By: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-Authored-By: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-Authored-By: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@jb-leger
Copy link
ContributorAuthor

jb-leger commentedJul 23, 2019
edited
Loading

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

@tacaswelltacaswell merged commitb2783ed intomatplotlib:masterJul 29, 2019
@jb-legerjb-leger deleted the logit_scale_stuff branchJuly 29, 2019 20:11
@jb-legerjb-leger mentioned this pull requestJul 30, 2019
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@tacaswelltacaswelltacaswell approved these changes

@anntzeranntzeranntzer approved these changes

Assignees

@tacaswelltacaswell

Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

4 participants
@jb-leger@anntzer@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp