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

MakeLogLocator respectnumticks#21177

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

Closed
dstansby wants to merge5 commits intomatplotlib:mainfromdstansby:LogLocator

Conversation

dstansby
Copy link
Member

@dstansbydstansby commentedSep 25, 2021
edited
Loading

PR Summary

Fixes#11518. The logic for determining decades seems really odd, and dates back all the way to 2013 (#1686). Previouslynumticks wasn't respected; this should now be fixed, as verified by the changes to the tests I've made wherenumticks=5.

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • [n/a] Conforms to Matplotlib style conventions (installflake8-docstrings and runflake8 --docstring-convention=all).
  • [n/a] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).

@dstansbydstansby changed the titleMakeLogLocator respectnumticksMakeLogLocator respectnumticksSep 25, 2021
@dstansbydstansbyforce-pushed theLogLocator branch 2 times, most recently from34d90dd to19938beCompareSeptember 25, 2021 14:43
@jklymak
Copy link
Member

Is this the same as#20950 ?

@dstansby
Copy link
MemberAuthor

Is this the same as#20950 ?

Nope, that's a PR to do with contouring, not with the locator itself.

jklymak reacted with thumbs up emoji

@dstansbydstansbyforce-pushed theLogLocator branch 2 times, most recently fromd345ea3 to90c11b5CompareSeptember 25, 2021 15:41
@dstansbydstansby marked this pull request as ready for reviewSeptember 25, 2021 15:52
@jklymak
Copy link
Member

I took a quick look at this, but I can't really tell from the description or tests what has changed and if it is truly an improvement or not.len(ticks) < numticks is the only test of the numticks that I can see, and that doesn't seem a very strict criteria. I'm also leery of the "improves numerical issues when vmin and vmax are on a decade". That shouldalways work, even if it means adding an epsilon to what is shown.

@dstansby
Copy link
MemberAuthor

len(ticks) < numticks is the only test of the numticks that I can see, and that doesn't seem a very strict criteria.

This isn't strict, but is what is promised in the docstring, and this PR fixes the locator to honour that promise.

I'm also leery of the "improves numerical issues when vmin and vmax are on a decade". That shouldalways work, even if it means adding an epsilon to what is shown.

I've added some extra tests to check that it works for different bases, and removed that comment.

jklymak reacted with thumbs up emoji

Copy link
Member

@jklymakjklymak left a comment

Choose a reason for hiding this comment

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

I guess this needs an API change note to warn folks their plots will be different.

I'm still a bit ignorant why this is abetter algorithm. It seems to make sense, but given that it will break folks it needs a strong justification?

# If we have decided that the stride is bigger than the range, clip
# the stride back to the available range - 1 with a floor of 1.
# This prevents getting axis with only 1 tick visible.
stride = max(1, numdec)
Copy link
Member

Choose a reason for hiding this comment

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

code cov thinks this needs a test, and since its new, it is probably correct....

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks for spotting - it turns out this bit of code is redundant now and never hit, so I've removed it.

@dstansby
Copy link
MemberAuthor

I've added an API change note. The tradeoff here is
Fixed bugs

  • maxticks is respected
  • The ticks on a log-scalederrorbar plot are now correct - see the third test image change.

Behaviour change

  • In some cases (two of our test cases that use log scales, see 1st/2nd test image change) an extra tick is added to a log-axis, but there is still plenty of room to display the tick values so (beyond being a change) this isn't an issue.

@jklymak
Copy link
Member

I guess the image changes are my point though; many of them have twice as many ticks as before, and it is not clear to me why that is "right" or desirable...

@dstansbydstansby mentioned this pull requestSep 30, 2021
7 tasks
@dstansby
Copy link
MemberAuthor

I don't think any have as much as twice as many ticks - maybe I missed something though? In some cases the stride increases by 1, but the test image changes show that this is a reasonable change as the ticks still fit comfortably along the Axis.

@jklymak
Copy link
Member

The last axes in the last image test is what particularly jumps out at me as suddenly having a lot more ticks. Maybe its fine though?

@dstansby
Copy link
MemberAuthor

You mean the one where minor ticks are now drawn? I think that's a bug fix - there's nothing in the tests that requests minor ticks not to be drawn, and I think it looks better for them being there.

@jklymak
Copy link
Member

They aren't drawn on master because the major ticks are 2 decades apart. With this change they are one decade apart.

@tacaswell

This comment has been minimized.

@jklymak
Copy link
Member

This needs a rebase...

Does anyone else have opinions about whether this is the correct algorithm or not? Its a breaking change, but certainly numticks should be better respected for log scales!

@dstansby
Copy link
MemberAuthor

Thanks - I'll wait on a third opinion before rebasing.

jklymak reacted with thumbs up emoji

@tacaswell
Copy link
Member

We talked about this on the call and have a couple of quetions:

  • do we have a case where we actually return more ticks than we promise (at the "will be shown" level)
  • can we get away with being clearer in the docstring about whatnumticks means (because major ticks can only go on decades this ends up being way over-constrained)
  • We were not convinced that two of the images that got more ticks actually looked better, but this may just be an issue of moving across a hard edge (how many decades the major tick steps are) and there is no way around putting the jumpsomeplace. Is it possible to tweak that logic a bit to make it earlier again?

@dstansby
Copy link
MemberAuthor

I've marked this as orphaned as I don't have the time/bandwidth to work more on this at the moment - if someone else wants to pick it up feel free.

@tacaswelltacaswell marked this pull request as draftJanuary 10, 2022 19:15
@dstansby
Copy link
MemberAuthor

I've put a large chunk of this in#24436, which is clearer and easier to review, so closing this.

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

@jklymakjklymakjklymak left review comments

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LogLocator ignores numticks without axis
4 participants
@dstansby@jklymak@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp