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

[Bug]: LogLocator with subs argument fragile. #24092

Closed
peterdsharpe opened this issue Oct 4, 2022 · 9 comments · Fixed by #25405
Closed

[Bug]: LogLocator with subs argument fragile. #24092

peterdsharpe opened this issue Oct 4, 2022 · 9 comments · Fixed by #25405
Labels
Difficulty: Hard https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Good first issue Open a pull request against these issues if there are no active ones! topic: ticks axis labels
Milestone

Comments

@peterdsharpe
Copy link

peterdsharpe commented Oct 4, 2022

Bug summary

Logarithmic tick markers do not appear if the y-axis scales a sufficient range, with the subs keyword argument of ticker.LogLocator set to non-trivial values.

Code for reproduction

import matplotlib.pyplot as plt
import matplotlib.ticker
import numpy as np

ll = matplotlib.ticker.LogLocator(subs=(1, 2, 5))

### The following code produces a plot with y-axis ticks at the expected locations.

fig, ax = plt.subplots()
x = np.arange(8)
plt.semilogy(x, 10 ** x)
ax.yaxis.set_major_locator(ll)
ax.yaxis.set_minor_locator(ll)
plt.title("Good Plot")
plt.show()

### The following code produces a plot with no y-axis ticks, which is unexpected and undesired.

fig, ax = plt.subplots()
x = np.arange(9)  # The only change is this line
plt.semilogy(x, 10 ** x)
ax.yaxis.set_major_locator(ll)
ax.yaxis.set_minor_locator(ll)
plt.title("Bad Plot")
plt.show()

### The problem is isolated to here, which returns correct values in the first case, but np.array([]) in the second case:
print(ll.tick_values(1, 1e7))
print(ll.tick_values(1, 1e8))

Actual outcome

image

image

Expected outcome

I expect to see ticks in both cases, as appears in the "Good Plot".

Additional information

The problem is isolated to ticker.LogLocator.tick_values(). This returns correct values in the first case (e.g., np.array([1.e-01 2.e-01 5.e-01 1.e+00 2.e+00 5.e+00 1.e+01 2.e+01 5.e+01 1.e+02 2.e+02 5.e+02])), but np.array([]) in the second case.

Operating system

Windows

Matplotlib Version

3.5.2

Matplotlib Backend

module://backend_interagg

Python version

3.9.13

Jupyter version

No response

Installation

conda

@jklymak jklymak changed the title [Bug]: [Bug]: LogLocator with subs argument fragile. Oct 4, 2022
@peterdsharpe
Copy link
Author

peterdsharpe commented Oct 13, 2022

Hi all, any thoughts on this? This is causing breaking behavior in a downstream application, and it'd be helpful to know whether Matplotlib maintainers think a fix will be quick, or if I should invest resources in working around this.

@jklymak
Copy link
Member

jklymak commented Oct 13, 2022

I think we would accept a fix if one were forthcoming. I suspect the is in the range where we tick every two decades and that is clashing with subs.

@tacaswell tacaswell added Good first issue Open a pull request against these issues if there are no active ones! Difficulty: Hard https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues labels Oct 13, 2022
@tacaswell
Copy link
Member

If you pan the "bad" example up or down you can make the ticks show up. They seem to alternate in and out based on the limits (not just the range) if you zoom out further they never appear.

There is clearly something very wrong in the logic of what sub does, but looking at the code I can not quickly understand it...

I'm labeling this as "good first issue" as I think the bug is clear (we should never not have tick labels!) but "hard" because the logic in the tick_values method is a bit convoluted (for a bunch of reasons, some historical, some because we are using the same code for major and minor ticks, some because people have very strong views about what log tick "should" be that are very conditional on the values involved). Any changes will have to be very careful about unintended consequences and understanding why the code was the way it was (likely will require some git/github archaeology) and adding a bunch more tests.

@tacaswell tacaswell added this to the v3.7.0 milestone Oct 13, 2022
@tacaswell
Copy link
Member

Also milestoning for 3.7 as we need to fix this, but I doubt (but we should check) that this is a regression in 3.6 and expect the fix to be somewhat high-risk so we should not backport it. If I am wrong about either of those, then we can re-milestone and backport.

@Hannibal404
Copy link

In ticker.py, as the difference between vmin and vmax increases, numdec increases and thus makes stride to be greater than 1 (in the else condition).

        stride = (max(math.ceil(numdec / (numticks - 1)), 1)
              if mpl.rcParams['_internal.classic_mode'] else
              (numdec + 1) // numticks + 1)

In the case of stride > 1, ticklocs gets assigned a blank array which I believe to be the root of the problem.

    if hasattr(self, '_transform'):
        ticklocs = self._transform.inverted().transform(decades)
        if have_subs:
            if stride == 1:
                ticklocs = np.ravel(np.outer(subs, ticklocs))
            else:
                # No ticklocs if we have >1 decade between major ticks.
                ticklocs = np.array([])

The ticks appearing upon panning may be explained by numticks increasing due to staggering and thus stride being reduced to 1 again, as upon plotting the bad plot for x = np.arange(10) even panning does not make the ticks appear.

Hope this helps. I would be glad to help fix this if you could guide me a little. Thanks

@Abitamim
Copy link

Abitamim commented Nov 1, 2022

I would like to tackle this. Does matplotlib assign issues or is it open for anyone to attempt to fix and submit pull requests?

@ksunden
Copy link
Member

ksunden commented Nov 1, 2022

@Abitamim we do not typically assign issues (sometimes core maintainers will self-assign as a reminder to themselves)

For more info see https://matplotlib.org/stable/devel/contributing.html#issues-for-new-contributors

@ksunden ksunden modified the milestones: v3.7.0, v3.7.1 Feb 14, 2023
haval0 added a commit to haval0/matplotlib that referenced this issue Mar 1, 2023
The test cases are based on the demonstration of the issue in upstream
issue matplotlib#24092

The test case for bad plot (test_tick_values_not_empty) should fail
until we implement our fix.

The test case for good plot (test_tick_values_correct) should already
suceed and continue to succeed after we implement our fix.
@QuLogic QuLogic modified the milestones: v3.7.1, v3.7.2 Mar 4, 2023
II-Day-II added a commit to haval0/matplotlib that referenced this issue Mar 6, 2023
II-Day-II added a commit to haval0/matplotlib that referenced this issue Mar 6, 2023
@II-Day-II
Copy link
Contributor

We are a group of students at KTH Royal Institute of Technology having a look at this as part of a course on software engineering.

Assuming the problem is only present in the case where mpl.rcParams['_internal.classic_mode'] is False, this commit seems to be the source. The floor division algebra appears to be incorrect, as the old while-loop method yields a stride of 1 with the parameters in the example above where the rewritten method yields 2.

Reverting to the old calculation (or an equivalent one) seems to fix the problem without causing any other tests to fail. We should have a PR ready soon.

II-Day-II added a commit to haval0/matplotlib that referenced this issue Mar 7, 2023
The attempt to simplify the stride calculation in `a06f343dee3cebf035b74f65ea00b8842af448e9` seems to be the cause of the problem. 
Using an approach equivalent to what was there before hopefully resolves matplotlib#24092.
@peterdsharpe
Copy link
Author

Yay! Thanks @II-Day-II - looking forward to this fix going live.

@QuLogic QuLogic modified the milestones: v3.7.2, v3.8.0 Mar 28, 2023
Higgs32584 pushed a commit to Higgs32584/matplotlib that referenced this issue Apr 17, 2023
The test cases are based on the demonstration of the issue in upstream
issue matplotlib#24092

The test case for bad plot (test_tick_values_not_empty) should fail
until we implement our fix.

The test case for good plot (test_tick_values_correct) should already
suceed and continue to succeed after we implement our fix.
Higgs32584 pushed a commit to Higgs32584/matplotlib that referenced this issue Apr 17, 2023
…otlib#3)

The attempt to simplify the stride calculation in `a06f343dee3cebf035b74f65ea00b8842af448e9` seems to be the cause of the problem. 
Using an approach equivalent to what was there before hopefully resolves matplotlib#24092.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: Hard https://matplotlib.org/devdocs/devel/contribute.html#good-first-issues Good first issue Open a pull request against these issues if there are no active ones! topic: ticks axis labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants