-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
cfbaeb9
to
943e025
Compare
I force push to add some more tests (because codedev was complaining) and I rebased on |
7d5e45b
to
9cb21df
Compare
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. |
Thanks :) |
02c4caf
to
5bdca67
Compare
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. |
There was a problem hiding this 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): |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks for your work on this @jb-leger ! |
There was a problem hiding this 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.
Thanks @tacaswell for your review. Sorry for the delay, I was away any kind of computers. |
Thanks @QuLogic for all your comments and suggestions. It was the first time I use the functionnality of github for applying suggestions as commits. |
73b7904
to
d81550e
Compare
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. |
…necessary anymore
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>
2335616
to
e113aff
Compare
Rebased to master for resolve conflict introduced by merge #14654. |
PR Summary
I had a lot of bugs on the labelling on
logit
scale. For example, the following code:Actual behavior
I obtain for the very large zoom:
For normal zoom:
For zoomed axis:
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:
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 :
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:Also, two little things:
use_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.