Thanks, I think that this makes sense! As seen, we use np.floor, just after round in the second failure. Although it may just be a coincidence.

Can you please add a test for this? I think a smoke test, where you basically plot something (simple) that currently triggers the error should be enough. With a comment that it is a smoke test and point to this PR for motivation. In this way we can make sure that the error doesn't return later if someone thinks "well, I can replace that np.round with round"...

One can of course discuss if actually all round should be np.round, like now, or if one should read the code a bit more careful and see if any can stay. On the other hand, one can also question if round is actually that much better for any reason? It is not so obvious from the code why sometimes round is chosen and sometimes np.round (except when we know that there is a vector involved).

Hey so I tried to reproduce the issue I experienced, with some np.inf data, and I didn't succeed. I also failed to retrieve the data that I had back then. I still think it is worth discussing with other developers, but I will leave my effort on this for now.

In principle I agree this makes sense, but there might be some other changes needed (like np.round returns floats where as round returns ints so we might need extra casts).

You can reproduce this with

import matplotlib.pyplot as plt
plt.ion()
fig, ax = plt.subplots()
import numpy as np
ax.loglog(np.logspace(0, 10, 10), np.logspace(0, 10, 10)**2)
ax.set_yticks([1, 10, np.inf])

which suggest that we should be doing better validation on the way into FixedLocator. Put another way, if we are getting np.inf here then there is very likely a bug (or user error) someplace else that we should deal with instead.

Wow thanks for creating this code example! I will put something similar in the tests I will hopefully write after discussing a solution.

Put another way, if we are getting np.inf here then there is very likely a bug (or user error) someplace else that we should deal with instead.

Definitely - having np.inf in your data is not scientific :), and perhaps a good explanatory warning should be printed when this is attempted. The minimum expectation is that matplotlib won't crash when it encounters such a situation.

Having np.inf in the data should be OK (we take care of this a bunch of places), it is asking for a tick at np.inf that is the problem.

OK I have an issue with running the tests locally, so I hope it will be OK with you that I will use the PR's CI to do that. Now I pushed a new test, without the fix yet, expecting to see the only test I added failing, and I will push a fix afterwards, expecting the issue to be fixed with it.

OK tests failures from CI:

=================================== FAILURES ===================================
_____________________________ test_yticks_with_inf _____________________________
[gw2] darwin -- Python 3.12.3 /Library/Frameworks/Python.framework/Versions/3.12/bin/python

    def test_yticks_with_inf():
        fig, ax = plt.subplots()
        ax.loglog(np.logspace(0, 10, 10), np.logspace(0, 10, 10)**2)
        ax.set_yticks([1, 10, np.inf])
>       fig.draw_without_rendering()

lib/matplotlib/tests/test_ticker.py:1837: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
lib/matplotlib/figure.py:3171: in draw_without_rendering
    self.draw(renderer)
lib/matplotlib/artist.py:95: in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
lib/matplotlib/artist.py:72: in draw_wrapper
    return draw(artist, renderer)
lib/matplotlib/figure.py:3155: in draw
    mimage._draw_list_compositing_images(
lib/matplotlib/image.py:132: in _draw_list_compositing_images
    a.draw(renderer)
lib/matplotlib/artist.py:72: in draw_wrapper
    return draw(artist, renderer)
lib/matplotlib/axes/_base.py:3113: in draw
    mimage._draw_list_compositing_images(
lib/matplotlib/image.py:132: in _draw_list_compositing_images
    a.draw(renderer)
lib/matplotlib/artist.py:72: in draw_wrapper
    return draw(artist, renderer)
lib/matplotlib/axis.py:1422: in draw
    ticks_to_draw = self._update_ticks()
lib/matplotlib/axis.py:1300: in _update_ticks
    major_labels = self.major.formatter.format_ticks(major_locs)
lib/matplotlib/ticker.py:217: in format_ticks
    return [self(value, i) for i, value in enumerate(values)]
lib/matplotlib/ticker.py:1088: in __call__
    is_x_decade = _is_close_to_int(fx)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

x = inf

    def _is_close_to_int(x):
>       return math.isclose(x, round(x))
E       OverflowError: cannot convert float infinity to integer

lib/matplotlib/ticker.py:2265: OverflowError
------------------------------ Captured log call -------------------------------

Now pushed the suggested fix. Hopefully CI will be green 🤞 .

Aish there are more errors then I anticipated... What if I will replace all math. functions with their numpy equivalent? Not only np.round.. Would that be accepted?

🎉 Managed to finally fix the new test I added (took me a while because of this). CI is green now. Your review is welcome.

BTW @tacaswell CI is green here as well for a while now :).

Can you try rebasing to see if we have a fix for the appevoyer failure on main?

Does this only let np.inf pass through as a value or does it also sort out/fix why a locator suggested putting a tick at inf?

Does this only let np.inf pass through as a value or does it also sort out/fix why a locator suggested putting a tick at inf?

I'm not sure I understood the 2nd part - what do you mean by "tick at inf"? If you are asking in general about the motivation for the PR, it is a bit hard for me to find the most accurate words to explain it, but for sure it fixes the test added in the 1st commit, and I believe it fixes many other potential issues with nan and infinite values.

I'm a bit hesitant to change all the math <-> np calls. This may have surprising side-effects. How much of this is to handle inf in set_yticks? I'd rather error there and keep other changes minimal.

In the original code that produced the issue I encountered and tried to fix here, I didn't explicitly used set_yticks with an np.inf in the argument, something internal in ticker.py made it try to set these yticks for the set of y values I had in my code. It is indeed unfortunate that I am not able now to get a list of such y values, but I can imagine it might happen given the usage of log to figure out where to put yticks.

Are you able to produce a minimal example of the issue we are trying to fix here? Without that, we're basically blind:

• we cannot judge whether the PR is the appropriate fix or whether another approach would be better
• we cannot add tests to ensure that the fixed behavior is not unintentionally reverted through future code changes

The proposed math <-> np change, is substantial (in particular because the changes are subtle). We'd need a very informed decision to go that direction. One prerequisite is that we exactly know what problem we are fixing with that.

Are you able to produce a minimal example of the issue we are trying to fix here?

I agree that the test I added here (suggested by @tacaswell in this comment) might not be very satisfying because it is a bit superficial, so I'm happy to report I did manage now to create a less superficial MWE :):

import matplotlib.pyplot as plt

fig, ax = plt.subplots()

ax.plot([1,2], [1, 1e261])
ax.set_yscale("log")
plt.show()

Gives math.isclose(x, round(x)) error right? Note how if you change the 261 to 260 you get no error. Another interesting thing to try is the following:

#!/usr/bin/env python

import numpy as np
import math

for oom in range(300, 4400):
    x = np.float128(10**oom)
    print("oom(x) = ", oom, np.log(x), math.log(x))

The above prints inf in the last word after oom = 309, and np.float128 fails after oom = 4300.

If I use np.float64 (Numpy's default float), it fails when oom = 309, exactly like the Python native float function.

Without that, we're basically blind:

• we cannot judge whether the PR is the appropriate fix or whether another approach would be better
• we cannot add tests to ensure that the fixed behavior is not unintentionally reverted through future code changes

The proposed math <-> np change, is substantial (in particular because the changes are subtle). We'd need a very informed decision to go that direction. One prerequisite is that we exactly know what problem we are fixing with that.

I agree overall with the development workflow you try to impose, but I find it a bit peculiar to justify the current state in which numpy/math functions are chosen arbitrarily (at least it seems like so). The PR simply suggests to be consistent, which is something I thought would be accepted even without a test, simply because it makes sense.

I can replace the test I added with the above MWE and verify that the PR fixes the test, but I want to know first what is the direction you'd like to take this now that I managed to obtain further clues as for the bug I experienced.

but I find it a bit peculiar to justify the current state in which numpy/math functions are chosen arbitrarily (at least it seems like so). The PR simply suggests to be consistent, which is something I thought would be accepted even without a test, simply because it makes sense.

Unfortunately, it's not that simple:

• There's no clear right or wrong for using math vs. numpy. The behavior is subtly different (which you actually rely on for making your case work). But we have no insight whether the changed behavior has undesired effects in other cases.
• Therefore, I'd try to minimize changes (if it ain't broken, don't fix it), which needs a clear insight why the current code is not suitable. -> Understand the problem and exactly solve that.

I can replace the test I added with the above MWE and verify that the PR fixes the test, but I want to know first what is the direction you'd like to take this now that I managed to obtain further clues as for the bug I experienced.

The next step is to follow the MWE code through a debugger and find out where the infinity comes in. There must be a place where 1e261 is turned into infinity, and most likely that's the code we have to fix.

Edit: Part of the issue is that we add whitespace around the data (aka. margin) so that data points are not at the edge of the Axes. On a log scale, that can be a significant numerical amount. To take that effect out, change the example to:

import matplotlib.pyplot as plt

fig, ax = plt.subplots()

ax.plot([1,2], [1, 1e269])
ax.set_yscale("log")
ax.margins(0)
plt.show()

which still works, but will fail for 1e270. Or even better, don't bother with data but isolate the topics to the axis limits

import matplotlib.pyplot as plt

fig, ax = plt.subplots()

ax.set_ylim(1, 1e270)  # works with 1e269
ax.set_yscale("log")
plt.show()

The next step is to follow the MWE code through a debugger and find out where the infinity comes in. There must be a place where 1e261 is turned into infinity, and most likely that's the code we have to fix.

OK, that's sort of what I did in the beginning, although I didn't have such a good MWE. I also described that process in the very first comment of this PR. In the meantime I reordered the commits a bit, and added a test that currently fails, as I have not yet found an optimal solution for it. Your view on this now is most welcome.

Given the recent discussion I'm going to move this to the 3.11 milestone.

The actual MWE is

from matplotlib.ticker import LogLocator
LogLocator().tick_values(0, 1e270)

and the relevant code:

decades is [-31 0 31 62 93 124 155 186 217 248 279 310] and with b=10.0 we end up on
ticklocs [1.e-031 1.e+000 1.e+031 1.e+062 1.e+093 1.e+124 1.e+155 1.e+186 1.e+217 1.e+248 1.e+279 inf]

with numpy 10.0 ** 310 == inf (pure python throws an OverflowError instead). Note that the largest number you can represent with 64-bit floats is ~ 1.8e308.

A tick locator should never place ticks at inf. The proper solution to this problem is to tune the tick location algorithm in LogLocator.tick_values().

The actual MWE is

from matplotlib.ticker import LogLocator
LogLocator().tick_values(0, 1e270)

and the relevant code:

matplotlib/lib/matplotlib/ticker.py

Lines 2463 to 2464 in 4468488

	decades = np.arange(math.floor(log_vmin) - stride,
	math.ceil(log_vmax) + 2 * stride, stride)

matplotlib/lib/matplotlib/ticker.py

Line 2473 in 4468488

ticklocs = b ** decades

decades is [-31 0 31 62 93 124 155 186 217 248 279 310] and with b=10.0 we end up on ticklocs [1.e-031 1.e+000 1.e+031 1.e+062 1.e+093 1.e+124 1.e+155 1.e+186 1.e+217 1.e+248 1.e+279 inf]

with numpy 10.0 ** 310 == inf (pure python throws an OverflowError instead). Note that the largest number you can represent with 64-bit floats is ~ 1.8e308.

A tick locator should never place ticks at inf. The proper solution to this problem is to tune the tick location algorithm in LogLocator.tick_values().

That's true, and it'd indeed be nice to start with writing such a test specifically for LogLocator, but even if that would be fixed, it is not clear to me what would be the next step. I tried:

diff --git i/lib/matplotlib/ticker.py w/lib/matplotlib/ticker.py
index 63a519f793..74cfac52e3 100644
--- i/lib/matplotlib/ticker.py
+++ w/lib/matplotlib/ticker.py
@@ -2401,7 +2401,7 @@ class LogLocator(Locator):
         have_subs = len(subs) > 1 or (len(subs) == 1 and subs[0] != 1.0)
 
         decades = np.arange(math.floor(log_vmin) - stride,
-                            math.ceil(log_vmax) + 2 * stride, stride)
+                            math.ceil(log_vmax) + 2 * stride, stride, dtype=np.float128)
 
         if have_subs:
             if stride == 1:

But then I encountered an issue in LogFormatterSciNotation, here:

Which makes fx be inf. That is also fixable by:

diff --git i/lib/matplotlib/ticker.py w/lib/matplotlib/ticker.py
index 74cfac52e3..5f3222fc1f 100644
--- i/lib/matplotlib/ticker.py
+++ w/lib/matplotlib/ticker.py
@@ -1088,7 +1088,7 @@ class LogFormatterMathtext(LogFormatter):
         b = self._base
 
         # only label the decades
-        fx = math.log(x) / math.log(b)
+        fx = np.log(x) / np.log(b)
         is_x_decade = _is_close_to_int(fx)
         exponent = round(fx) if is_x_decade else np.floor(fx)
         coeff = round(b ** (fx - exponent))

And then the original test_LogFormatter_almost_inf passes. However, it is not that satisfying, as I'm not capable of explaining up to what number will np.float128 be good enough. And also equally importantly, I don't know how to test LogLocator's decades calculation without changing too many inner parts there. Not only that, if I use 1e400 in test_LogFormatter_almost_inf, it won't fail but because 1e400 is already treated as inf, and it shouldn't, although np.float128 is used when decades are computed...

This rabbit hole is what made me use numpy's functions all the time, as they seem a bit more accurate almost all of the times. If we'll after all still go in such approach, it'd should be a good idea to use float128 all the time.

Rewriting for float128 would be a non-trivial medium-sized task. Most users don't use float 128 anyway and introducing it only to have larger ticklabels feels overkill - or at least a completely separate discussion with much larger impact.

I'd rather try and manipulate the ticker so that it does not produce tick label positions beyond 1e308. The very naive approach would be to clip the nan position. One has to try whether that looks ok. Otherwise, you'd need to come up with a better decades algorithm for the edge case, e.g. choose something between vmax and 3e8 as the top tick position and a suitable spacing going down in regular decades.

Apparently the maximal float size is actually 1.7976931348623157e+308, and can be computed like this:

import sys
import numpy as np
print(np.finfo(np.float64).max)
print(sys.float_info.max)

Rewriting for float128 would be a non-trivial medium-sized task. Most users don't use float 128 anyway and introducing it only to have larger ticklabels feels overkill - or at least a completely separate discussion with much larger impact.

I think, that the natural thing to do would be to inherit the dtype (float64 or float128) of the user's input. If they use float128 because they know they have very large floats, we shouldn't disallow them to do so. If they use integers OTH (which can grow infinitely large), we should disallow them to use integers bigger then the maximum of float128. Perhaps some of this behavior should be handled at the level of Locator.

Indeed that's a non-trivial task though, so I'm not sure I'd be able to invest time in it. TBH, the issue I encountered here is not an active issue for me anymore. I just kept giving this attention because I want to contribute back to Matplotlib. I think that the 3 commits prior to the last one are worth merging either way, and we can create a separate issue with all the details we have aggregated in the last few days.

I'd rather try and manipulate the ticker so that it does not produce tick label positions beyond 1e308.

I think that silently ignoring such inputs can be confusing for the users, so I'd raise an error much earlier.

I think, that the natural thing to do would be to inherit the dtype (float64 or float128) of the user's input.

AFAIK, we don't have any guarantees on that, and it would be a massive undertaking. Basically, you'd have to make sure that all internal calculations do respect this. I don't think we can or should go there in the foreseeable future.

I'd rather try and manipulate the ticker so that it does not produce tick label positions beyond 1e308.

I think that silently ignoring such inputs can be confusing for the users, so I'd raise an error much earlier.

I'm not ignoring inputs. The inputs are the data range. The TickLocator tries to invent ticks at nice positions. And results in current ticks in below image. The naive solution would be to clip the invented right-most tick. The axis limits would still cover the whole data. The proposed alternative dcades algorithm could try to find the solution for alternative ticks.