| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I have mixed feelings about this, so far. The 'tick1On' etc. attributes are ugly but fairly deeply embedded. Is this the time to make such an invasive change? Or would it be just as well to defer it for a more thorough overhaul of the overly complex and sluggish Tick/Axis/Spine system? Maybe we will never get to that, though. |
Sorry, something went wrong.
|
I don't think the system is that deeply embedded (but it has been around for a while, for sure). The deprecation is basically just replacing the attributes by calls to set_visible throughout; the only complication is with code that proxies these attributes using properties (i.e., the skew axes example), but this kind of code is always going to interact in a tricky manner with deprecations (because we arguably can't cover all possible ways in which our attributes have been proxied...). |
Sorry, something went wrong.
|
I do not disagree it simplifies things, but I am not sure it is worth the disruption in this case. attn @astrofrog Does this cause issues for WCSAxes? |
Sorry, something went wrong.
|
The changes for SkewT seem sensible to me. I hadn't seen ExitStack before. Certainly I'm a bit 👍 on having one way to handle things (i.e. set_visible) across the library. Would this work today if I stole the draw() implementation? |
Sorry, something went wrong.
|
Should work, but haven't tried :) |
Sorry, something went wrong.
|
I don't think this will cause issues for WCSAxes, though it will require some updates in APLpy (https://github.com/aplpy/aplpy/blob/eb09ec39506db2da91f92a383016d1328df731c3/aplpy/colorbar.py#L133) but I can live with that. |
Sorry, something went wrong.
|
Not going to insist on marking this as RC, but still bumping the suggestion up. |
Sorry, something went wrong.
|
I am ok with this in principle, needs a rebase and version updates. |
Sorry, something went wrong.
|
done |
Sorry, something went wrong.
|
There are a number of cases (most notably update_position() and draw()) that were previously only executed conditionally on the visibility. I agree that updating always is the more correct thing to do (otherwise the value is not correct when enabling visibility later on.). On the other hand, I'm a bit concerned of the performance impact. Have you evaluated this? |
Sorry, something went wrong.
|
I haven't, but I also think that it is the correct thing to do. Obviously it would be possible to hide them back again under a check for get_visible() but the semantics would be so-slightly different (if someone was really relying on that) and I don't think it's a good feature to keep anyways... |
Sorry, something went wrong.
There was a problem hiding this comment.
Seems fine, except for the ExitStack context stuff, which I don't follow, and hence is maybe a bit obtuse for a user example.
Does this have a slight behaviour change. Before if we set things while the label was not on it didn't have an affect. Now the label is not visible, but you do change the properties. So if the user turns the labels on again, they will get different results I think with this PR than before. OTOH I think the results they will get w/ this PR are more correct, so I'm OK w/ the change.
Sorry, something went wrong.
| def label2On(self, value): | ||
| self._label2On = value | ||
| def draw(self, renderer): | ||
| with ExitStack() as stack: |
There was a problem hiding this comment.
This is kind of mysterious for an example w/o a comment why you need it.
Sorry, something went wrong.
There was a problem hiding this comment.
added an explanation, I hope it helps
Sorry, something went wrong.
|
CI failures unrelated... @efiring, you expressed misgivings. Are you placated, or ?? If so, I'll merge... |
Sorry, something went wrong.
|
OK to merge once it passes the tests. |
Sorry, something went wrong.
|
@anntzer after a rebase feel free to merge once it passes tests.... |
Sorry, something went wrong.
Visibility attributes should just be set on the underlying artists (`tick.gridline.set_visible`, etc.) rather than maintaining yet another layer of control.
|
Done (modulo appveyor which is queued, but there's no reason for it to fail :p). |
Sorry, something went wrong.
This will hit every call to seaborn.heatmap, which is a pretty common function inside seaborn. |
Sorry, something went wrong.
|
will it be a problem for seaborn to replace by label1? |
Sorry, something went wrong.
|
I mean it used to be a simple alias, self.label = self.label1. So that shouldn't be a problem. I just wanted to make sure we are aware of the implication that once 3.1 is released thousands of seaborn users will get some warning message. |
Sorry, something went wrong.
|
Good point. We shouldn't do that. |
Sorry, something went wrong.
The Tick.label property will be deprecated and begin warning in a future version of Matplotlib. Explicitly using the correct property will avoid this. See matplotlib/matplotlib#10088 and matplotlib/matplotlib#13631
| Back | FazBrowse Home | New Git URL |
Visibility attributes should just be set on the underlying artists
(tick.gridline.set_visible, etc.) rather than maintaining yet another
layer of control.
Note that this technically breaks code that overrode the gridOn etc. attributes using properties (such as the old implementation of skewed axes) but I can't think of a reasonable way to emit a warning for these...
PR Summary
PR Checklist