| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
|
||
| self._lastNumMajorTicks = numticks | ||
| ticks = self.majorTicks[:numticks] | ||
| while len(self.majorTicks) < numticks: |
There was a problem hiding this comment.
Did we decide that the _lastNumMajorTicks logic wasn't needed? What did all that do and why can we remove it? Certainly your change is simpler, but it seems most of the un-simpleness in the old code was because of the _lastNumMajorTicks logic.
Sorry, something went wrong.
There was a problem hiding this comment.
It's private, unused anywhere else, and, to the best of my understanding, this implementation does the same thing...
It was introduced in 08610fc and never touched since.
Sorry, something went wrong.
|
Thinking again, the purpose of the whole mechanism seems to be to manage an numticks-length sublist of the currently used ticks. Is tick creation that expensive so that it's beneficial or necessary to keep them around for later use? The whole mechanism looks like a premature optimization to me. I also do not understand when copy_tick_props is neccessary. Currently it's performed if self._lastNumMajorTicks < numticks, i.e. when numticks has increased. In such a case, it does not play a role if the additional ticks were added or reused. It's only applied to the additional ticks. This does not make any sense. I don't know if desired tick properties can change. If not, it would be sufficient to copy the properties on creation (N.B.: This is what this PR does). If they can change, one would also have to update the existing ticks below _lastNumMajorTicks (none of the two implementations does this). So in short, the PR does not do exactly the same, but the current implementation is not completely reasonable anyway. It all comes down to if the properties can change later on. If they cannot, the PR is correct. |
Sorry, something went wrong.
|
Tick properties can be changed by Axes.tick_params / Axis.set_tick_params. |
Sorry, something went wrong.
|
Semi OT: Probably majorTicks (and minorTicks) should be made private. People should use get_major_ticks to be sure to receive a correctly maintained list. AFAIK majorTicks is not documented officially. If backward compatibility is desired nevertheless, majorTicks can become a property with a deprecation warning, still returning _majorTicks. |
Sorry, something went wrong.
|
Tick instantiation is costly enough that we're going through hoops to make that lazy... |
Sorry, something went wrong.
|
There (the lazy stuff), whole sets of ticks were created multiple times for each Axes instantiation. The code here just gives a benefit if the tick number is decreased and then increased again for the same axis. IMO this is rarely the case. Maybe in interactive plots. In which time is particularly critical, so you could still be right. |
Sorry, something went wrong.
|
I don't mind getting rid of it (assuming the speed cost is OK), but that's not the point of this PR, which, to the best of my knowledge, does exactly the same thing as before, just in fewer lines and in a less convoluted (IMO) fashion. |
Sorry, something went wrong.
|
Ok, back on topic: As discussed above, it does not do exactly the same: it does not call copy_tick_props for existing ticks that get reused. If that is neccessary is a point of discussion. If I understand @QuLogic correctly, properties can change and should be updated. Not tested, but something like this could be handled incorrectly: ax.set_major_locator(MaxNLocator(5)) ax.get_major_ticks() ax.set_major_locator(MaxNLocator(2)) ax.get_major_ticks() ax.set_tick_params(something) ax.set_major_locator(MaxNLocator(4)) ax.get_major_ticks() # returned ticks are not updated |
Sorry, something went wrong.
|
The original version only called copy_tick_props to for i in range(self._lastNumMajorTicks, len(self.majorTicks)), so (AFAICT, but may have missed something) only the new ticks. |
Sorry, something went wrong.
|
After some testing, I've come to the conclusion that this PR indeed works as one would expect and the original code just did some unnecessary work. |
Sorry, something went wrong.
|
One additional question, that does not necessarily have to be targeted in this PR: Would it make sense to restore the descriptors in reset_ticks? We currently have two possible paths of initialization: The descriptor and reset_ticks. I realize that set_clip_path in reset_ticks would directly instantiate the descriptor again. But I don't understand the details and the stale mechanism therein. Just wanted to bring this to attention. If the additional change is not reasonable or requires a lot of other changes, don't hold up this PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
This works for me, and is indeed simpler.
Sorry, something went wrong.
|
@timhoffm reset_ticks does reset the descriptors. try: del self.majorTicks except AttributeError: pass only succeeds if the majorTicks are set on the instance dict (rather than the class dict) which occurs when the descriptor has already been triggered. After a successful del, majorTicks is removed from the instance dict and the next attribute access will again go to the class dict and trigger the descriptor. |
Sorry, something went wrong.
|
@anntzer Sorry, I've been looking at the code of another branch. You're right of course. |
Sorry, something went wrong.
|
Something went wrong ... Please have a look at my logs. |
Sorry, something went wrong.
|
? There's no 2.2 branch. |
Sorry, something went wrong.
|
hummmsorry was not paying attention. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Extracted from #9727 and its review.
PR Checklist