Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix axes aspect for non-linear, non-log, possibly mixed-scale axes.#14727
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
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.
This seems good to me. I wonder if both functions should be deprecated and a new function made private. Its not clear that this function is needed by the user and its output has the potential to be confusing.
I explained above why I'm not deprecating it: it doesn't help for cases where a third-party subclass overrides that method (which is explicitly documented as overridable). |
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.
One minor question, and one style disagreement that I feel pretty strongly about; otherwise, I think it is good.
lib/matplotlib/tests/test_image.py Outdated
| Z = np.zeros((10, 10)) | ||
| Z[::2] = 1 | ||
| fig, ax = plt.subplots() | ||
| ax.imshow(Z, extent=[1, 100, 1, 100], cmap='viridis', | ||
| vmax=1, vmin=-1) | ||
| ax.set_yscale('log') | ||
| ax.set(yscale='log', aspect='auto') |
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.
Why does this test need to be changed? Does this imply that user code will also need to be changed?
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 moved theaspect="auto" to the imshow() call, but basically, this was previously relying on aspect=1 (set by imshow) not being respected in semilog plots (hence the recwarn just above).
So yes, if someone was setting aspect=1 (either explicitly, or implicitly via imshow()) and also hoping that this would be ignored in semilog plots, then that won't work anymore. But that's sort of the point of the PR...
lib/matplotlib/axes/_base.py Outdated
| pb = position.frozen() | ||
| pb1 = pb.shrunk_to_aspect(box_aspect, pb, fig_aspect) | ||
| self._set_position(pb1.anchored(self.get_anchor(), pb), 'active') | ||
| return |
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 more readable the way it was, with the return here. By saving that one line, you are forcing the reader to look through along block of lines to be sure that in fact there is no more to be done in the "box" case. And that long block is indented a level, when it wouldn't need to be.
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 don't really agree -- as it was written previously, it is not so obvious to figure out where the adjustable == "datalim" case is handled (well, at least I was puzzled by it). I don't mind putting back the return here (doing it now) so that it is clear nothing else is to be done, but I would still leave the bottom block indented (which actually doesn't push any single line beyond the 79-character limit).
lib/matplotlib/axes/_base.py Outdated
| if aspect_scale_mode == "log": | ||
| xmin, xmax = math.log10(xmin), math.log10(xmax) | ||
| ymin, ymax = math.log10(ymin), math.log10(ymax) | ||
| elif self._adjustable == 'datalim': |
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.
Suggestion: replace theelif line with a comment, "# self._adjustable is 'datalim'". Then the following indentation is not needed. As it stands, theelif line is misleading because it implies that there is an alternative--which there isn't.
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.
ok, done
The main change is to make Axes.get_data_ratio take axes scales intoaccount. This is a breaking change in get_data_ratio, but also the mostreasonable way I could think of to implement the feature while alsosupporting third-party Axes subclasses that override this method (giventhat it is explicitly documented as being overridable for this purpose).(Compare, for example, with a patch that also deprecates get_data_ratioand moves the whole computation to apply_aspect; now what do we do withthird-party overrides?)Also move the adjustable="datalim"-part of the implementation ofapply_aspect down one indentation block for symmetry withadjustable="box".The change in test_log_scale_image is because we can't rely on aspect=1not being implemented for semilog plots anymore...
QuLogic commentedJul 18, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@anntzer You need a "Closes" for every issue if you want them all to close. |
Uh oh!
There was an error while loading.Please reload this page.
The main change is to make Axes.get_data_ratio take axes scales into
account. This is a breaking change in get_data_ratio, but also the most
reasonable way I could think of to implement the feature while also
supporting third-party Axes subclasses that override this method (given
that it is explicitly documented as being overridable for this purpose).
(Compare, for example, with a patch that also deprecates get_data_ratio
and moves the whole computation to apply_aspect; now what do we do with
third-party overrides?)
Also move the adjustable="datalim"-part of the implementation of
apply_aspect down one indentation block for symmetry with
adjustable="box".
The change in test_log_scale_image is because we can't rely on aspect=1
not being implemented for semilog plots anymore...
Closes#4900,#8878,#13819.
attn@efiring (who suggested the idea of keeping get_data_ratio though with a new behavior) as we discussed this during the call.
PR Summary
PR Checklist