Skip to content

Improve symlog axis ticks#27310

Open
schtandard wants to merge 7 commits intomatplotlib:mainfrom
schtandard:symlog_ticks
Open

Improve symlog axis ticks#27310
schtandard wants to merge 7 commits intomatplotlib:mainfrom
schtandard:symlog_ticks

Conversation

@schtandard
Copy link
Copy Markdown

@schtandard schtandard commented Nov 12, 2023

PR summary

In the current implementation, symlog axes only get major ticks by default. Depending on the data range, this can lead to too few or even no ticks at all. When "subticks" between decades are specified manually, those are sometimes labeled in an unintended manner (when they are at a decade).

This PR re-implements the tick placement and formatting for symlog axes to remedy these problems. If I am not mistaken, this closes #17402, closes #18757 and closes #25994.

I am submitting this as a PR now to get some general feedback on the approach and pointers if I need to consider anything regarding style or conventions that I missed. If I get a thumbs up, I would polish some things off (like making _SymmetricalLogUtil's methods work with np.arrays and have .firstdec() cache results) before merging.

My approach for the tick locator

I was aiming for SymmetricalLogLocator to behave identically to LogLocator as long as the data range is outside of the linear part of the scale and extend this reasonably to the linear part if it's present. What I came up with is this (I'm describing the positive half axis here for simplicity, everything works the same on the negative side):

  • Major ticks at decades, where the first decade to be considered for labelling must be at least half the length of a decade from 0 (otherwise there would be infinitely many labeled decades close to 0).
  • That decade receives the decade number 1, the next larger one 2 and so on. 0 gets the "decade" number 0. Values between those decades get interpolated decade numbers.
  • We now have an obvious definition of the number of decades in the data range and can basically copy the logic from LogLocator, the only caveat being that 0 should always receive a tick if it is in the range.
  • In the linear part of the scale, we add the same minor ticks below the first decade as below any other and add the next lower decade as a minor tick. That is, if with base b one gets minor ticks 2 to b-1 between larger decades one gets 1 to b-1 below the first one, effectively producing linear ticks between the first decade and 0.

LogFormatter (which is also used for symlog axes) is adapted accordingly.

  • I added a separate field _firstsublabels for placing sublabels below the first decade.
  • As with log axes it can sometimes happen that there is only one major tick with unlabeled minor ticks around it. This is fine (one can still identify the unlabeled positions) as long as the major tick is at some decade and not at 0, which can happen with symlog axes. In that case, we need to label at least one of the minor ticks to allow reading values off the axis.

Since both the locator and the formatter need the calculation of the first decade I placed the code regarding this into a helper class _SymmetricalLogUtil.

Finally, SymmetricalLogScale is adapted to actually use the new functionality by default.

I checked all parameter ranges I could think of and this seems to work out quite nicely.

This is how it looks.

showcase_new

For comparison, this is how it used to look.

showcase_current

Code to reproduce.
import numpy as np
from matplotlib import pyplot as plt

def showcase_scales(axs, *plotargs, **plotkwargs):
    for ax, scale in zip(axs, ['linear', 'symlog', 'log']):
        ax.set_yscale(scale)
        ax.yaxis.grid()
        ax.plot(*plotargs, **plotkwargs)

values = [
    np.linspace(3, 150),
    np.linspace(20, 60),
    np.linspace(0.5, 12),
    np.linspace(0.5, 5),
    np.linspace(0.6, 3),
    np.linspace(1e-4, 1e-2),
    np.linspace(0, 40),
    np.linspace(-0.8, 0.9),
    np.linspace(-4, 4),
    np.linspace(-22, 22),
    np.linspace(0, 0),
    np.linspace(1, 1),
    np.linspace(2, 2),
    np.linspace(10, 10),
    np.linspace(13, 13),
]
fig, axs = plt.subplots(len(values), 3, squeeze=False, sharex=True,
                        figsize=(9, 2 * len(values)))
for a, v in zip(axs, values):
    showcase_scales(a, v)
axs[0, 0].set_title('linear')
axs[0, 1].set_title('symlog')
axs[0, 2].set_title('log')
fig.tight_layout()
fig.savefig('showcase.png')

Requested feedback

There are some details regarding which I would like feedback/guidance from you.

  • When computing stride (i.e. the number of decades between major ticks) a possible forced tick at 0 is not considered. This may lead to an off-by-one error regarding numticks (i.e. a stride value smaller by one would have been possible) but I feel like preventing this is not worth the complication of taking it into account. Do you agree?
  • subs=None is equivalent to subs='auto' in LogLocator but to subs=(1.0,) in SymmetricalLogLocator. Is that on purpose? I left it like that.
  • For some reason, base came after linthresh in SymmetricalLogLocator's signature, contrary to most other occurences. In adding linscale I reordered those. While this is strictly a backwards incompatible change (whereas I just appended linscale everywhere else), I don't expect it to have ever been used in an incompatible way: providing base only make sense when transform (the first argument) is not given, so it's most probably always been don by keyword argument, though it is in principle possible to say SymmetricalLogLocator(None, None, 2, 10) to get SymmetricalLogLocator(linthresh=2, base=10). Should I switch it back for backwards compatibility or do we care more about consistent argument order?
  • LogFormatter threw away the sign of any value, even though it is used for negative values with symlog axes. This was no problem because LogFormatterMathtext overwrites __call__ and preserves the sign, so it didn't actually show up on any axis (with the default configuration). I fixed this. Is it fine to just leave it in this PR or should it be a separate one?
  • In test_ticker.py the test TestSymmetricalLogLocator.test_extending() relies on rcParams['axes.autolimit_mode'] being set to round_numbers but this is not set up in the test itself. I assume that this configuration is left in place by some previous test, though I can't tell where. Is this by design or should an rc_context be used, like in some other tests?
  • The checklist below tells me to note API changes (which the new linscale argument to the locator and the formatter as well as the changed default value of SymmetricalLogScale's subs argument qualify as, I think) with a directive and release note, but the linked section does not seem to exist. What do I need to do?

PR checklist

@schtandard
Copy link
Copy Markdown
Author

schtandard commented Nov 12, 2023

Ah, about the tests: Not all tests ran succesfully on my machine, but all the ones that fail also do so on main, so I'm confident this is not due to my changes.

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@schtandard schtandard marked this pull request as ready for review November 12, 2023 19:44
@schtandard
Copy link
Copy Markdown
Author

Leaving this comment to ping you guys, as it's been more than a week since I opened this PR. :-)

@schtandard
Copy link
Copy Markdown
Author

Leaving another ping-comment, as it's been almost six weeks.

@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label Jan 4, 2024
@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Jan 4, 2024

Sorry there hasn't been much discussion here. I've put this on the agenda for tomorrow's meeting.

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Jan 11, 2024

Sorry I meant to comment last week after the meeting, but it seemed to have been left here without going through.

In general, we are in favour of this change. However, we would like to see that it be behind a flag of some sort, either a) an argument to the class init, or b) an rcParam, or c) both. Sometime in the future, it may make sense to flip that flag (which I would be in favour of), but it should be tested out a bit first.

  • When computing stride (i.e. the number of decades between major ticks) a possible forced tick at 0 is not considered. This may lead to an off-by-one error regarding numticks (i.e. a stride value smaller by one would have been possible) but I feel like preventing this is not worth the complication of taking it into account. Do you agree?

I think I've seen an issue related to this somewhere and will try to find it.

  • subs=None is equivalent to subs='auto' in LogLocator but to subs=(1.0,) in SymmetricalLogLocator. Is that on purpose? I left it like that.

It was probably an artifact of whoever implemented things, but then was refactored to share code and now it seems inconsistent. But now it can't easily change for backwards compatibility reasons.

  • For some reason, base came after linthresh in SymmetricalLogLocator's signature, contrary to most other occurences. In adding linscale I reordered those. While this is strictly a backwards incompatible change (whereas I just appended linscale everywhere else), I don't expect it to have ever been used in an incompatible way: providing base only make sense when transform (the first argument) is not given, so it's most probably always been don by keyword argument, though it is in principle possible to say SymmetricalLogLocator(None, None, 2, 10) to get SymmetricalLogLocator(linthresh=2, base=10). Should I switch it back for backwards compatibility or do we care more about consistent argument order?

Unfortunately we do, so it will have to be reverted. In some cases, we might want to start making arguments keyword-only, but that does require going through a deprecation cycle.

  • LogFormatter threw away the sign of any value, even though it is used for negative values with symlog axes. This was no problem because LogFormatterMathtext overwrites __call__ and preserves the sign, so it didn't actually show up on any axis (with the default configuration). I fixed this. Is it fine to just leave it in this PR or should it be a separate one?

It's probably fine here, but if you have some easy way to show that difference outside of the other changes, it might be quicker for review as a separate PR.

  • In test_ticker.py the test TestSymmetricalLogLocator.test_extending() relies on rcParams['axes.autolimit_mode'] being set to round_numbers but this is not set up in the test itself. I assume that this configuration is left in place by some previous test, though I can't tell where. Is this by design or should an rc_context be used, like in some other tests?

This is set by the classic style, which is the default for all tests, though new tests should start using mpl20 and in that case you would need to set the rcParam yourself. There's not really a need for rc_context unless you are mixing more than one call in a test, as all rcParams are reset before each test.

  • The checklist below tells me to note API changes (which the new linscale argument to the locator and the formatter as well as the changed default value of SymmetricalLogScale's subs argument qualify as, I think) with a directive and release note, but the linked section does not seem to exist. What do I need to do?

I think this might have moved here. @story645 I guess we need to update the pull request template for some of the recent refactors?

@story645
Copy link
Copy Markdown
Member

The checklist below tells me to note API changes (which the new linscale argument to the locator and the formatter as well as the changed default value of SymmetricalLogScale's subs argument qualify as, I think) with a directive and release note, but the linked section does not seem to exist. What do I need to do?

is something like this clearer?

image

if not or you have a suggestion, please let me know at #27641

@schtandard
Copy link
Copy Markdown
Author

Thanks for your comments! I think I have everything to make final adjustments now. I'm quite busy at the moment, but I will try and get that done soon.

@rcomer
Copy link
Copy Markdown
Member

rcomer commented Jan 13, 2024

Does this also close #10369?

@melissawm
Copy link
Copy Markdown
Member

Hi @schtandard , are you interested in finishing this up? If you need help, let us know. Cheers!

@melissawm melissawm moved this to Waiting for author in First Time Contributors Mar 6, 2026
@schtandard
Copy link
Copy Markdown
Author

Hi @melissawm, thanks for the ping and sorry for the long delay; a stressfull period of my life dragged on longer than I had expected. It finally concluded last month, though, so I hope I'll now be able to wrap this up. In fact, I already took a quick look recently to see what's left to do. I'll have another crack and try to incorporate all the things mentioned above, hopefully without as much delay this time.

- Use the same logic to calculate numticks in LogFormatter.set_locs() as
  later in LogLocator.tick_values().
- Do not hard-code n_request == 9 in minor tick check.
- Correct a type in an explanatary comment (< instead of <=).
- Actually calculate the largest possible stride, as outlined in the
  comments.
- The check for lonely ticks at the end of LogLocator.tick_values()
  assumed that major ticks are not also minor ticks. Fix that logic.
The new SymmetricalLogLocator emulates the behavior of LogLocator in the
logarithmic region and extends its behavior to the linear part in a
(hopefully) reasonable manner.

The change is hidden behind a new rcParam (and corresponding class
parameters) for now.
- For SymLogLocator, do not use a hard-coded limit preventing minor
  ticks. Instead, always use the same behavior.
- For LogLocator, revert to the old behavior with the hard-coded limit
  of 10 available major ticks.
@schtandard
Copy link
Copy Markdown
Author

schtandard commented Mar 16, 2026

Alright, I gave it another go. The tick placement for log axes had changed since I first tackled this, so I basically re-wrote the symlog stuff to match. I re-named the old branch to symlog_ticks_old, in case you want to have a look, though I don't think it is of interest anymore. Some notes:

  • As requested, I hid the new behavior behind a new rcParam axes.formatter.legacy_symlog_ticker. I was a bit unsure about the name and category. Let me know if I should rename it.
  • SymmetricLogScale() now has the default value subs='auto'.
    • With the previous default subs=None there are no minor ticks, which is bad.
    • Note that SymmetricalLogLocator interprets subs=None as "no minor ticks" while LogLocator treats it as subs='auto'. The latter is for "consistency with previous bad API" according to a comment in the code.
    • So, while this is not quite backward-compatible, I think it's the cleanest way to go. What do you think?
  • Also, SymmetricLogScale() now does not suppress minor tick labels. This I could hide behind the new rcParam if you want.
  • I thought about having _SymmetricalLogUtil.firstdec() cache its result, as it is needed quite a bit. However, I was now unsure if the efficiency is worth the added code complexity. What do you think?

No documentation changes or release notes are included, yet. Once the API is set, I will have a look at what's necessary there.

This is how it looks. showcase_new
And here is the "legacy" version. showcase_legacy
For comparison, this is how it used to look (identical to legacy version). showcase_current
Code to reproduce.
import numpy as np
import matplotlib as mpl
from matplotlib import pyplot as plt

mpl.rcParams['axes.formatter.legacy_symlog_ticker'] = False

def showcase_scales(axs, *plotargs, **plotkwargs):
    for ax, scale in zip(axs, ['linear', 'symlog', 'log']):
        ax.set_yscale(scale)
        ax.yaxis.grid()
        ax.plot(*plotargs, **plotkwargs)

values = [
    np.linspace(3, 150),
    np.linspace(20, 60),
    np.linspace(0.5, 12),
    np.linspace(0.5, 5),
    np.linspace(0.6, 3),
    np.linspace(1e-4, 1e-2),
    np.linspace(0, 40),
    np.linspace(-0.8, 0.9),
    np.linspace(-4, 4),
    np.linspace(-22, 22),
    np.linspace(0, 0),
    np.linspace(1, 1),
    np.linspace(2, 2),
    np.linspace(10, 10),
    np.linspace(13, 13),
]
fig, axs = plt.subplots(len(values), 3, squeeze=False, sharex=True,
                        figsize=(9, 2 * len(values)))
for a, v in zip(axs, values):
    showcase_scales(a, v)
axs[0, 0].set_title('linear')
axs[0, 1].set_title('symlog')
axs[0, 2].set_title('log')
fig.savefig('showcase.png', bbox_inches='tight')

@melissawm melissawm moved this from Waiting for author to Needs review in First Time Contributors Mar 16, 2026
@timhoffm
Copy link
Copy Markdown
Member

I would like to discuss whether we need a flag and the legacy behavior. Do we make / want to make guarantees on tick placement? Does anybody depend on reproducible exact positioning of ticks?

#29698 has been merged as a strict improvement without the ability to keep the old behavior. We should consider doing the same here (and probably mention this change here in the #29698 what's new message).

@schtandard
Copy link
Copy Markdown
Author

I would like to discuss whether we need a flag and the legacy behavior.

I'm not sure that I am meant to participate in this discussion, but since there have been no comments so far, let me note that I agree that this could be merged as a strict improvement much like #29698. Removing the relevant class parameters and rcParam is easy and would simplify the code a bit.

@QuLogic has your reasoning from back in the day changed or do you still see reasons to hide the change behind a flag for now?

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Apr 3, 2026

That was the conclusion from the meeting; I do not recall the discussion at the time for why, and unfortunately the meeting notes do not have any detail.

I'll note that @timhoffm was not in that meeting; perhaps we should re-discuss this at the next meeting?

@timhoffm
Copy link
Copy Markdown
Member

timhoffm commented Apr 3, 2026

Sure, we can discuss. I’ll put my opinion in already here.

So far we have done several changes to ticks without backs compatibility options, e.g. #29698, #29054, #18754, #13069 (and possibly more).

We haven’t seen any complaints on these improvements/fixes of tick positions so likely nobody relies on them (or they can cope with changes). Therefore, flags just for the sake of keeping old behavior in tick locations seem like unnecessary complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Needs review

8 participants