Repository navigation
Fix axes aspect for non-linear, non-log, possibly mixed-scale axes. - #14727
efiring merged 1 commit into
Conversation
Sorry, something went wrong.
jklymak
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
efiring
left a comment
There was a problem hiding this comment.
One minor question, and one style disagreement that I feel pretty strongly about; otherwise, I think it is good.
Sorry, something went wrong.
| 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.
Why does this test need to be changed? Does this imply that user code will also need to be changed?
Sorry, something went wrong.
There was a problem hiding this comment.
I moved the aspect="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...
Sorry, something went wrong.
| 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.
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 a long 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.
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
| 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.
Suggestion: replace the elif line with a comment, "# self._adjustable is 'datalim'". Then the following indentation is not needed. As it stands, the elif line is misleading because it implies that there is an alternative--which there isn't.
Sorry, something went wrong.
There was a problem hiding this comment.
ok, done
Sorry, something went wrong.
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...
|
@anntzer You need a "Closes" for every issue if you want them all to close. |
Sorry, something went wrong.
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