| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I think the change on ticker.py's side should look something like diff --git i/lib/matplotlib/ticker.py w/lib/matplotlib/ticker.py
index 89e077a04..84bd9a2b4 100644
--- i/lib/matplotlib/ticker.py
+++ w/lib/matplotlib/ticker.py
@@ -2093,9 +2093,7 @@ def is_decade(x, base=10):
def is_close_to_int(x):
- if not np.isfinite(x):
- return False
- return abs(x - round(x)) < 1e-10
+ return abs(x - np.round(x)) < 1e-10
class LogLocator(Locator):
@@ -2256,7 +2254,11 @@ class LogLocator(Locator):
# If we're a minor locator *that expects at least two ticks per
# decade* and the major locator stride is 1 and there's no more
# than one minor tick, switch to AutoLocator.
- return AutoLocator().tick_values(vmin, vmax)
+ ticklocs = AutoLocator().tick_values(vmin, vmax)
+ # Don't overstrike the major labels.
+ ticklocs = ticklocs[
+ ~is_close_to_int(np.log(ticklocs) / np.log(b))]
+ return ticklocs
return self.raise_if_exceeds(ticklocs)
def view_limits(self, vmin, vmax):
after which I don't know if the other changes (touching labelOnlyBase) are needed? |
Sorry, something went wrong.
|
Yeah that might be better. Not sure we should be implying that a locator is a minor locator. My preference would be that we just explicitly pass the major locator instance to anything we want to be a minor locator and then do a set diff between the two locators Or simply doing this in the axis code after the fact where we know about both locators. |
Sorry, something went wrong.
|
The log locator code already has "magic" to guess whether it's a minor or a major locator; given that it's already assuming that it works we may as well rely on it. |
Sorry, something went wrong.
|
I'd argue that the "magic" is probably a bad idea because it relies on the user knowing the magic when they specify the Locator, and as we have seen, even the developers don't get the "magic" right. I think there are two approaches - 1) tell the locator explicitly what ticks to avoid with an optional major_ticks=None kwarg, or 2) get the ticks from both locators, and then exclude any overlaps in the minor ticks. I guess I think 1) is the best because I can think of cases where the minorLocator might want to exclude a tick because it is close but not equal to a major tick. I don't see why we can't impliment this as a Locator._remove_duplicates method that defaults on all Locators to removing all the duplicates in the __call__. If child classes want to ignore all this they can, so I don't think it'd be a backwards incompatibility issue. |
Sorry, something went wrong.
|
(BTW implimenting your change to squash the bug, the other discussion is more long-term) |
Sorry, something went wrong.
| return AutoLocator().tick_values(vmin, vmax) | ||
| ticklocs = AutoLocator().tick_values(vmin, vmax) | ||
| # Don't overstrike the major labels. | ||
| ticklocs = [t for t in ticklocs if |
There was a problem hiding this comment.
eh, that needs to use boolean indexing, no?
Sorry, something went wrong.
There was a problem hiding this comment.
Not sure what you mean? Your version didn't work because ticklocs is an array and is_close_to_int only works on a single value. This seems to work...
Sorry, something went wrong.
There was a problem hiding this comment.
My patch also fixed is_close_to_int to be vectorized: it deleted the lines
- if not np.isfinite(x): - return False
you may have missed that?
(if x is not finite, the expression below will also be false).
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, my fault.
Sorry, something went wrong.
I would name this exclude_ticks=None, which is more general. It may take a list of values and/or (possibly also later implemented) a locator. |
Sorry, something went wrong.
| # Don't overstrike the major labels. | ||
| ticklocs = ticklocs[ | ||
| ~is_close_to_int(np.log(ticklocs) / np.log(b))] | ||
| print('ticklocs', ticklocs) |
There was a problem hiding this comment.
remove this line
Sorry, something went wrong.
| ticklocs = b ** decades | ||
|
|
||
| _log.debug('ticklocs %r', ticklocs) | ||
| print('subs', subs, have_subs, ticklocs) |
There was a problem hiding this comment.
remove this line
Sorry, something went wrong.
There was a problem hiding this comment.
Oooops, sorry
Sorry, something went wrong.
There was a problem hiding this comment.
I guess that can count as Jody's approval of my patch :)
Sorry, something went wrong.
| ticklocs = AutoLocator().tick_values(vmin, vmax) | ||
| # Don't overstrike the major labels. | ||
| ticklocs = ticklocs[ | ||
| ~is_close_to_int(np.log(ticklocs) / np.log(b))] |
There was a problem hiding this comment.
This looks like a hard-coded assumption on the position of the major ticks. Will this fail if I’ve set non-default values no the major locator?
Sorry, something went wrong.
There was a problem hiding this comment.
Absolutely. See discussion above for fixing that. But that assumption is already in the code, this just makes that assumption work properly under all conditions. The next step is to get rid of the assumption, but thats a bigger project.
Sorry, something went wrong.
There was a problem hiding this comment.
Ok, that's reasonable for now. However, it would be good to explicitly document this in the code with a comment (unless you're immediately following up with the full fix).
Sorry, something went wrong.
There was a problem hiding this comment.
Sure thing
Sorry, something went wrong.
There was a problem hiding this comment.
Anybody can merge after CI pass.
Sorry, something went wrong.
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
$ git checkout v3.0.x $ git pull
$ git cherry-pick -m1 71dd3ae125d060bc08193abb93cca643245e3de0
$ git commit -am 'Backport PR #13126: FIX: minor log ticks overwrite'
git push YOURFORK v3.0.x:auto-backport-of-pr-13126-on-v3.0.x
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
Sorry, something went wrong.
|
Automatic backport fails due to #12865 not being backported. We should either backport both or none. |
Sorry, something went wrong.
|
Moved both to 3.1. The logic around the log ticks is subtle and probably better to keep changes to it for minor release (not patch releases). |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
attempting to fix log overprint issue (closes #13112)
Before:
After
PR Checklist