Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
34d90dd
to19938be
CompareIs this the same as#20950 ? |
Nope, that's a PR to do with contouring, not with the locator itself. |
d345ea3
to90c11b5
CompareI 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. |
This isn't strict, but is what is promised in the docstring, and this PR fixes the locator to honour that promise.
I've added some extra tests to check that it works for different bases, and removed that comment. |
There was a problem hiding this 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?
lib/matplotlib/ticker.py Outdated
# 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) |
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
I've added an API change note. The tradeoff here is
Behaviour change
|
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... |
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. |
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? |
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. |
They aren't drawn on master because the major ticks are 2 decades apart. With this change they are one decade apart. |
This comment has been minimized.
This comment has been minimized.
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! |
Thanks - I'll wait on a third opinion before rebasing. |
We talked about this on the call and have a couple of quetions:
|
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. |
I've put a large chunk of this in#24436, which is clearer and easier to review, so closing this. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Fixes#11518. The logic for determining decades seems really odd, and dates back all the way to 2013 (#1686). Previously
numticks
wasn't respected; this should now be fixed, as verified by the changes to the tests I've made wherenumticks=5
.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).