Deprecate JPEG-specific kwargs and rcParams to savefig. #16231
Conversation
615d649 to
bfd3005
Compare
6dfdb97 to
c06dbda
Compare
There was a problem hiding this comment.
_delete_parameter is quite intricate, even more so with the added kwargs handling. Please try to be as clear as possible in the documentation and implementation.
Note: This part is actually the one which held me from reviewing for a while. Maybe it would have helped a bit if it was it's own PR. Even though the jpg deprecation would still be dependent on it, the change to _delete_parameter would have been a smaller PR, which might have been a psychological advantage for the motivation to review.
lib/matplotlib/cbook/deprecation.py
Outdated
|
|
||
| signature = inspect.signature(func) | ||
| assert name in signature.parameters, ( | ||
| kw_name = next((param.name for param in signature.parameters.values() |
There was a problem hiding this comment.
| kw_name = next((param.name for param in signature.parameters.values() | |
| # name of kwargs parameter, e.g. 'kwargs' if **kwargs is in the signature of the | |
| # decorated function, or None if the function does not have such a parameter | |
| kwargs_name = next((param.name for param in signature.parameters.values() |
Even though this may seem picky, this is rather unusual code and every bit of clarity helps (connection kwargs <-> kwargs_name).
lib/matplotlib/cbook/deprecation.py
Outdated
| @@ -307,7 +307,7 @@ def __repr__(self): | |||
| _deprecated_parameter = _deprecated_parameter_class() | |||
|
|
|||
|
|
|||
| def _delete_parameter(since, name, func=None): | |||
| def _delete_parameter(since, name, func=None, **kwargs): | |||
There was a problem hiding this comment.
I would explicitly mention in the docstring:
This can also be used for parameters that are handled through
**kwargs.
| addendum = ( | ||
| f"If any parameter follows {name!r}, they should be passed as " | ||
| f"keyword, not positionally.") | ||
| if kwargs.get("addendum"): |
There was a problem hiding this comment.
AFAICS this is a new optional parameter of _delete_parameter. Please make it explicit.
There was a problem hiding this comment.
I now mentioned that all additional parameters are forwarded to warn_deprecated.
a09585a to
84cf1e2
Compare
|
I agree it's not so nice to bundle the changes to _delete_parameter here, but OTOH, my experience tells me that if I made them as a separate PR with no intended use case, it would have taken forever to get it reviewed... |
Just speaking from my point of view: I'd been more inclined to review this separately. You could have explained the need in a short sentence and immediately created the second PR with "waiting for other PR" tagged. Yes, sometimes PR take a long time, in particular if they are involved. But I dispute that embedding them in another PR speeds up review (compared to a PR that indicates the context as proposed). |
|
Thanks for your suggestion (which I do value a lot, given that you are a reviewer on most of my PRs so I do want to keep you happy :)) |
|
added tests for delete_parameter (as a separate commit) |
|
rebased |
|
@anntzer I propose you rebase and self-merge deprecation PRs once they got two approvals. Otherwise, we'll be boundcing back and forth with "Needs rebase" tags. |
Saving Matplotlib figures to jpeg is generally not a great idea to start with (even at the default quality of 95 there are visible (faint) artefacts around sharp lines, and at quality=95 the files produced are bigger than the corresponding pngs anyways). We don't need to completely get rid of jpeg support, but we can at least simplify the code path (which otherwise also needs to be duplicated in mplcairo) and not have to document these jpeg-specific kwargs (in two places!). Note that users can still set them via `pil_kwargs`. The changes to _delete_parameter are so that we can write ``` @_delete_parameter(..., "foo") def f(**kwargs): ... ``` where `foo` actually only shows up in `kwargs`.
|
Sure, I'll do that. |
Saving Matplotlib figures to jpeg is generally not a great idea to start
with (even at the default quality of 95 there are visible (faint)
artefacts around sharp lines, and at quality=95 the files produced are
bigger than the corresponding pngs anyways).
We don't need to completely get rid of jpeg support, but we can at least
simplify the code path (which otherwise also needs to be duplicated in
mplcairo) and not have to document these jpeg-specific kwargs (in two
places!). Note that users can still set them via
pil_kwargs.The changes to _delete_parameter are so that we can write
where
fooactually only shows up inkwargs.PR Summary
PR Checklist