-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Deprecate frameon kwarg and rcParam to savefig. #11692
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate frameon kwarg and rcParam to savefig. #11692
Conversation
lib/matplotlib/__init__.py
Outdated
_deprecated_remain_as_none = { | ||
'axes.hold': ('2.1',), |
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.
(other mentions of axes.hold have already been removed)
8a519c6
to
9d3f5a6
Compare
I think the reason for these "remain_as_none" is so that people with style
files that can be used across multiple versions of matplotlib won't get
errors if they happen to have an entry that was recently deprecated.
…On Wed, Jul 18, 2018 at 9:22 AM, Antony Lee ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/matplotlib/__init__.py
<#11692 (comment)>
:
> _deprecated_remain_as_none = {
- 'axes.hold': ('2.1',),
(other mentions of axes.hold have already been removed)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#11692 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-NsToeuw4eppaMusOIeTvBOjtfykks5uHzaUgaJpZM4VUlrv>
.
|
But one cannot actually set it to a value of None from the rcfile (https://github.com/matplotlib/matplotlib/blob/v2.2.2/lib/matplotlib/rcsetup.py#L151). So in practice, the only change removing that entry does is to replace a specific deprecation message ("axes.hold is deprecated, will be removed in 3.0") by a generic unknown-key message ("Bad key "axes.hold""). |
9d3f5a6
to
1b1fbf2
Compare
Look at that code again. If it is None, then it returns None. It doesn't go
through validate_bool().
…On Wed, Jul 18, 2018 at 10:29 AM, Antony Lee ***@***.***> wrote:
But one cannot actually set it to a value of None from the rcfile (
https://github.com/matplotlib/matplotlib/blob/v2.
2.2/lib/matplotlib/rcsetup.py#L151).
So in practice, the only change removing that entry does is to replace a
specific deprecation message ("axes.hold is deprecated, will be removed in
3.0") by a generic unknown-key message ("Bad key "axes.hold"").
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11692 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-DTXKMm2RNfoI1qkcmfVIa8evnWXks5uH0ZBgaJpZM4VUlrv>
.
|
But the rc parser will pass the string "None", not the None object. Manually adding such an entry in your rc file confirms that behavior. |
1b1fbf2
to
57269b9
Compare
Hmm, you are right. Then what's the point of all of this stuff in
`__init__.py`? We have deprecation mechanisms in multiple places! It is
obvious that we were supposed to be able to suppress deprecation messages
for axes.hold in some places, but not others? That's weird.
…On Wed, Jul 18, 2018 at 10:51 AM, Antony Lee ***@***.***> wrote:
But the rc parser will pass the string "none", not the None object.
Manually adding such an entry in your rc file confirms that behavior.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#11692 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-FledSgMNyWTIi-Hmes3eFvqDvf1ks5uH0tlgaJpZM4VUlrv>
.
|
One may want to restore the suppression behavior somehow (but that's a separate issue), but right now the only point is to trigger a deprecation message on get and set (and when doing |
As pointed out in #10939 I would expect there to be a way to get rid of the "frame", i.e. the figure's background patch entirely. |
Why does a transparent rectangle in the output file bother you? |
There are several reasons not wanting a background at all. Let's simply say transparency is unfortunately not handled well in every program you might use your figure with afterwards. Indeed,
gives the expected result. But that is somehow inconsistent. If the figure patch is the object to manipulate for the saved figure's background appearance, one would expect that to be the case in general, not only for I.e. the following does not produce a green background,
|
Not sure I'm following here, but there sure are reasons to let the saved background be different from the one shown on screen, and to do this within the One could of course make that more consistent, in the sense of |
I'd rather have the API proposed in #10023 (comment). Let's move the discussion there? |
57269b9
to
b69f8e6
Compare
The following API elements are deprecated: | ||
|
||
- the ``frameon`` kwarg to ``savefig`` and the ``savefig.frameon`` rcParam (use | ||
``facecolor`` instead), |
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.
Can you please be a bit more specific? What values does one have to pass to facecolor
to obtain the effect of frameon=True/False
.
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.
clarified
b69f8e6
to
d8a126c
Compare
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.
Anybody can merge after CI pass.
``frameon = False`` has *never* worked since its introduction in 2013 due to the buggy implementation that only took it into account if True: if frameon: original_frameon = self.get_frameon() self.set_frameon(frameon) (despite the doc stating otherwise). One can also set the facecolor kwarg or savefig.facecolor rcParam to fully transparent for the same effect.
d8a126c
to
897c497
Compare
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.
Blocking on further discussion tomorrow on the call. I think @ImportanceOfBeingErnest has some valid points and we are going to get a lot of blowback about this.
Do you have a specific example in mind? |
Going to merge this, but am open to reverting if we get much push back on this after it is released. I think running this through controlling the color / transparency of the patch directly is clearer than an independent flag. |
…692-on-v3.1.x Backport PR #11692 on branch v3.1.x (Deprecate frameon kwarg and rcParam to savefig.)
frameon = False
has never worked since its introduction in 2013 (#1875)due to the buggy implementation that only took it into account if True:
(despite the doc stating otherwise). One can also set the facecolor
kwarg or savefig.facecolor rcParam to fully transparent for the same
effect.
(See also #10023.)
PR Summary
PR Checklist