Skip to content
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

set offset threshold to 4 #7104

Closed
story645 opened this issue Sep 13, 2016 · 20 comments
Closed

set offset threshold to 4 #7104

story645 opened this issue Sep 13, 2016 · 20 comments
Labels
API: default changes Difficulty: Easy https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Milestone

Comments

@story645
Copy link
Member

story645 commented Sep 13, 2016

Using the code from #7084, ax.set_xlim(1990, 2020) yields
index

Tried this for some other values and it seems broken for everything except values that follow the form %d*10^n so 10, 20, 300, etc.

Eta this also yields weirdness: ax.plot(range(1990,2020), range(1990,2020))
index

@story645 story645 changed the title set_xlim only works for values of the form c*10^n set_xlim only works for values of the form %d10^n Sep 13, 2016
@efiring
Copy link
Member

efiring commented Sep 13, 2016

Please put in exactly the code snippets that yield the two example images, and elaborate on what it is that looks wrong. I can't tell from what you have here so far.

@story645
Copy link
Member Author

story645 commented Sep 13, 2016

Sorry about that:

fig, ax = plt.subplots()
ax.plot(range(1990,2020), range(1990,2020))

and it's not a range bug, 'cause the following yields the same figure:

fig, ax = plt.subplots()
ax.plot(list(range(1990,2020)), list(range(1990, 2020)))

This is version 2.0.0b3.post2245+g4f559a9

The issue is that the xticks and xlims aren't mapping to the datavalues, which are between 1990 and 2020. It should look something like this (which I got by manually setting the labels):
index

@story645 story645 changed the title set_xlim only works for values of the form %d10^n data values mapping to x-values incorrectly Sep 13, 2016
@story645 story645 changed the title data values mapping to x-values incorrectly data values mapping to ticks incorrectly Sep 13, 2016
@efiring
Copy link
Member

efiring commented Sep 13, 2016

I don't see anything wrong, except in your manually-adjusted example. The Formatter is using an offset of 2000, so the tick labels are from 2000 - 10 to 2000 + 20. In many cases like this one doesn't actually want an offset. It can be turned off with rcParams['axes.formatter.useoffset'] = False, or ax.ticklabel_format(useOffset) = False.

@LindyBalboa
Copy link
Contributor

As efiring stated, this is relatively standard behavior, albeit rather counterintuitive. I don't know the source code well enough without looking; could there be room for improvement in the default behavior of deciding when to turn on offsetting?

@story645
Copy link
Member Author

story645 commented Sep 13, 2016

I fully fully support changing the default 'cause that offset seems so incredibly arbitrary and like you said, way counterintuitive. I think it looks at base 10 for defaults somehow, since something like ax.plot(range(200,400)) yields ticks between 200 and 400.

Also, @efiring how come calling ax.set_xlim((1990,2020)) doesn't overwrite the offset?

@efiring
Copy link
Member

efiring commented Sep 13, 2016

On 2016/09/13 9:15 AM, hannah wrote:

Also, @efiring https://github.com/efiring how come calling
ax.set_xlim((1990,2020)) doesn't overwrite the offset?

Setting the view limits is independent of the Formatter; the offset
comes from the Formatter.

@story645
Copy link
Member Author

@efiring I just realized that half my issue is that I didn't read the notation correctly. Why is the default to pitch it to scientific notation for 4 numbers? (What is the cut off?)

@efiring
Copy link
Member

efiring commented Sep 13, 2016

On 2016/09/13 9:27 AM, hannah wrote:

Why is the default to pitch it to scientific notation for 4 numbers?
(What is the cut off?)

A non-zero offset is used if it saves at least 2 significant digits in
the tick labels, according to the comment in
ScalarFormatter._compute_offset(). There is no API for changing that
threshold when offsets are used.

It looks like the offset is always being forced into scientific
notation. This does seem like a misfeature. Maybe the offset should be
formatted using the same powerlimits criterion as is used for the tick
labels.

@story645
Copy link
Member Author

Maybe the offset should be formatted using the same powerlimits criterion as is used for the tick labels.

I'm 👍 on that.

@story645 story645 changed the title data values mapping to ticks incorrectly tick offset defauolts to scientific notation Sep 13, 2016
@story645 story645 changed the title tick offset defauolts to scientific notation tick offset defaults to scientific notation Sep 13, 2016
@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Sep 13, 2016
@tacaswell
Copy link
Member

@anntzer was doing some work on this, but I do not remember if it got merged (sorry).

There is also #6086 which is still waiting for a review (sorry again @VincentVandalon)

@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2016

The choice of the offset value (not its formatting) was changed by myself in #5785.

The formatting of the offset value is addressed by #5804 (so that the offset displays enough significant digits to match the ticks themselves, and so that the cursor values are also displayed in a useful manner), but this PR also lumps in a large amount of breaking API changes to the formatter class (which I still think seriously need some API cleanup). TBH I have lost interest in making this backwards compatible, I think it's too difficult to be worth it and now simply disable any use of offsets from my matplotlibrc. (I can update my PR if we agree to break backcompat but will probably not add a complete backcompat layer.)

@VincentVandalon
Copy link

VincentVandalon commented Sep 14, 2016

No problem @tacaswell we are all doing this on a voluntary basis (I think).

In #6086 i tried to implement the desired behavior with as few as possible changes/refactoring. When you reviewed the pull request, you (rightly so) commented that it was not clear from the code if the desired behavior was achieved. The code was not transparent to begin with, and it did not improve with my changes.

I started refactoring to make the code clear, but that overlapped with #5804 and #5785. So #6068 ended up in limbo.

If the ticker.py code has undergone refactoring in the mean time, we could close #6086

@efiring
Copy link
Member

efiring commented Oct 10, 2016

@story645 do you see a way out of the impasse here? If not, I think this needs to be booted down the road to v2.1.

@anntzer
Copy link
Contributor

anntzer commented Oct 10, 2016

There's a relevant discussion of the formatting of the offset value in the first three messages of #5804, FWIW.

@tacaswell
Copy link
Member

Lets bump the threshold up to 3 or 4 digits to make years behave better.

There are enough questions on SO specifically related to this issue with years (and I have seen it go wrong for my co workers) is worth fixing.

@anntzer
Copy link
Contributor

anntzer commented Oct 10, 2016

Ah, I didn't think about that use case. Seems reasonable to me.

@story645
Copy link
Member Author

story645 commented Oct 13, 2016

@efiring I'm for whatever solution is easy and more user friendly. I support @tacaswell's suggestion of making a hard minimum of 4 digits since years are very common use case, so much so that it's the usecase that triggered my opening this issue.

@tacaswell tacaswell changed the title tick offset defaults to scientific notation set offset threshold to 4 Oct 19, 2016
@tacaswell tacaswell added the Difficulty: Easy https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues label Oct 19, 2016
@tacaswell
Copy link
Member

https://github.com/matplotlib/matplotlib/blob/v2.x/lib/matplotlib/ticker.py#L693 is the expression that needs to change, someone just needs to understand it enough to make the threshold 4 instead of 2....

@efiring
Copy link
Member

efiring commented Oct 23, 2016

In the line below that, I would try changing to this:

 if abs_max // 10 ** oom >= 1000

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 31, 2016
efiring added a commit to efiring/matplotlib that referenced this issue Oct 31, 2016
@dopplershift
Copy link
Contributor

Fixed by #7365.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: default changes Difficulty: Easy https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

No branches or pull requests

7 participants