Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cfbaeb9
to943e025
CompareI force push to add some more tests (because codedev was complaining) and I rebased on |
7d5e45b
to9cb21df
CompareOk, 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 :) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
02c4caf
to5bdca67
CompareThanks@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. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 beatol
instead ofrtol
?
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 the1-N
format only from0.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 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 |
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 oldLogitLocator
which has aminor
attribute.
Thanks for your work on this@jb-leger ! |
QuLogic left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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. |
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. |
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 commentedJul 23, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Rebased to master for resolve conflict introduced by merge#14654. |
Uh oh!
There was an error while loading.Please reload this page.
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.