Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Reuse scale from sharing axis when calling cla().#12831
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
| thresh: The degree above which to crop the data. | ||
| """ | ||
| mscale.ScaleBase.__init__(self) |
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.
Even thoughScaleBase.__init__ does not exist currently, I think we should nevertheless include asuper().__init__(). This generally good code style and leaves us with the chance of introducingScaleBase.__init__ later without breaking derieved scale classes.
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.
sure
Thisseems right to me, if sharing means two axises have the same scales (which I think they should). |
Right now (before this PR) they "do", except that they don't after a call to cla() if the scale constructor takes additional arguments, as the scale constructor for the shared axes won't get these additional arguments. Which is what this PR is fixing. |
The previous code, which called mscale.scale_factory(self._sharex.xaxis.get_scale(), self.xaxis)assumed that this would be sufficient to "copy" a scale object, but infact this fails to copy e.g. the base of log scales, so the log basewould be reset to its default (=10).Instead, just share the scale object, as is done when *creating* ashared axis.This implies that Scales should be reusable across multiple Axis, hencethe added note in the docstring. (Note that right now, LogScale objectsuse the info of whether the axis is an `x` or a `y` axis to decidewhether to request arguments named `basex` or `basey`; it would notbe too difficult to change the signature of LogScale to always take a`base` argument instead (which would be a more reasonable signaturetoo...); the main problem would be the API difference with `loglog(...,basex=..., basey=...)` -- but on the other hand it's not as if Scaleswere commonly instantiated by end users (for example one cannot evenpass a Scale object to `set_scale`(!))).
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 think this is better than the current behaviour. We should also discuss deprecating the axis kwarg/property, since I'm not convinced its used, and can't really imagine its use.
In the middle of my long commit message there's the explanation for the (silly, IMO) use of the axis argument:
The other problem is that this would also break third-party scale classes, e.g.https://github.com/matplotlib/mpl-probscale/blob/master/probscale/probscale.py#L95. I guess we could implement rather silly introspecting code to figure out whether the Scale implements the old or the new signature... Other than these "practical" issues I agree with getting rid of the axis argument. |
The previous code, which called
assumed that this would be sufficient to "copy" a scale object, but in
fact this fails to copy e.g. the base of log scales, so the log base
would be reset to its default (=10).
Instead, just share the scale object, as is done whencreating a
shared axis.
This implies that Scales should be reusable across multiple Axis, hence
the added note in the docstring. (Note that right now, LogScale objects
use the info of whether the axis is an
xor ayaxis to decidewhether to request arguments named
basexorbasey; it would notbe too difficult to change the signature of LogScale to always take a
baseargument instead (which would be a more reasonable signaturetoo...); the main problem would be the API difference with
loglog(..., basex=..., basey=...)-- but on the other hand it's not as if Scaleswere commonly instantiated by end users (for example one cannot even
pass a Scale object to
set_scale(!))).Helps a bit for#12819; at least now the bottom right plot goes to using log-2 instead of log-10 after the reset.
PR Summary
PR Checklist