Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Improve symlog axis ticks#27310

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

Open
schtandard wants to merge18 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromschtandard:symlog_ticks

Conversation

schtandard
Copy link

@schtandardschtandard commentedNov 12, 2023
edited
Loading

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 forsymlog axes to remedy these problems. If I am not mistaken, thiscloses#17402,closes#18757 andcloses#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 withnp.arrays and have.firstdec() cache results) before merging.

My approach for the tick locator

I was aiming forSymmetricalLogLocator to behave identically toLogLocator 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 thedecade 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 fromLogLocator, 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 forsymlog axes) is adapted accordingly.

  • I added a separate field_firstsublabels for placing sublabels below the first decade.
  • As withlog 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 withsymlog 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.
importnumpyasnpfrommatplotlibimportpyplotaspltdefshowcase_scales(axs,*plotargs,**plotkwargs):forax,scaleinzip(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)))fora,vinzip(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 computingstride (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 regardingnumticks (i.e. astride 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 tosubs='auto' inLogLocator but tosubs=(1.0,) inSymmetricalLogLocator. Is that on purpose? I left it like that.
  • For some reason,base cameafterlinthresh inSymmetricalLogLocator's signature, contrary to most other occurences. In addinglinscale I reordered those. While this is strictly a backwards incompatible change (whereas I just appendedlinscale everywhere else), I don't expect it to have ever been used in an incompatible way: providingbase only make sense whentransform (the first argument) is not given, so it's most probably always been don by keyword argument, though it is in principle possible to saySymmetricalLogLocator(None, None, 2, 10) to getSymmetricalLogLocator(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 withsymlog axes. This was no problem becauseLogFormatterMathtext 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?
  • Intest_ticker.py the testTestSymmetricalLogLocator.test_extending() relies onrcParams['axes.autolimit_mode'] being set toround_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 anrc_context be used, like in some other tests?
  • The checklist below tells me to note API changes (which the newlinscale argument to the locator and the formatter as well as the changed default value ofSymmetricalLogScale'ssubs argument qualify as, I think) with adirective and release note, but the linked section does not seem to exist. What do I need to do?

PR checklist

The new locator is based on LogLocator in such a way that it should giveidentical results if the data is only in a logarithmic part of the axis.It is extended to the linear part in a (hopefully) reasonable manner.
We will use it in the formatter, too.
Very cramped ticks are already avoided by choice of the first decade.
@schtandard
Copy link
Author

schtandard commentedNov 12, 2023
edited
Loading

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

Copy link

@github-actionsgithub-actionsbot 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 uson gitter for real-time discussion.

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

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

@schtandardschtandard marked this pull request as ready for reviewNovember 12, 2023 19:44
@schtandard
Copy link
Author

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

@schtandard
Copy link
Author

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

@QuLogicQuLogic added the status: needs comment/discussionneeds consensus on next step labelJan 4, 2024
@QuLogic
Copy link
Member

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

schtandard reacted with thumbs up emoji

@QuLogic
Copy link
Member

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 computingstride (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 regardingnumticks (i.e. astride 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 tosubs='auto' inLogLocator but tosubs=(1.0,) inSymmetricalLogLocator. 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 cameafterlinthresh inSymmetricalLogLocator's signature, contrary to most other occurences. In addinglinscale I reordered those. While this is strictly a backwards incompatible change (whereas I just appendedlinscale everywhere else), I don't expect it to have ever been used in an incompatible way: providingbase only make sense whentransform (the first argument) is not given, so it's most probably always been don by keyword argument, though it is in principle possible to saySymmetricalLogLocator(None, None, 2, 10) to getSymmetricalLogLocator(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 withsymlog axes. This was no problem becauseLogFormatterMathtext 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.

  • Intest_ticker.py the testTestSymmetricalLogLocator.test_extending() relies onrcParams['axes.autolimit_mode'] being set toround_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 anrc_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 usingmpl20 and in that case you would need to set thercParam yourself. There's not really a need forrc_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 newlinscale argument to the locator and the formatter as well as the changed default value ofSymmetricalLogScale'ssubs argument qualify as, I think) with adirective and release note, but the linked section does not seem to exist. What do I need to do?

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

story645 reacted with eyes emoji

@story645
Copy link
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 adirective 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
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
Member

Does this also close#10369?

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

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@Sma6500Sma6500Sma6500 approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
6 participants
@schtandard@QuLogic@story645@rcomer@Sma6500@dstansby

[8]ページ先頭

©2009-2025 Movatter.jp