Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Error out when unsupported kwargs are passed to Scale.#14879
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
No deprecation period, both because6357245 likewise added the exceptionfor LogScale without deprecation, and because this is by far most likelyto be catching bugs rather than real uses.
Is this something we've decided? Because there are lots of places where we don't check for valid kwargs. |
If you look at the linked issues it's pretty clearly a good way to shoot oneself in the foot. |
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 fully approve that we should not silently accept unused kwargs.
For disallowing without deprecation, I’m mildly worried to break user code. Even if it’s a bug, the output has apparently been sufficiently correct for the user. It might be friendlier to just warn (you are providing incorrect args) instead of hard breaking. Maybe this could use a shortened deprecation period of just one release? But not fighting over this either way.
Breaking user code is not so much an issue as breaking downstream packages. No developper is safe from using the API in an unintended way and writing seemingly sensical code. So as usual with API changes, a deprecation seems useful (similar to how matplotlib would appreciate upstream packages doing the same). |
I... don't understand what "sufficiently correct for the user" means. If you look at the linked issues, I am quite confident that there must be users who are currently passing linthresy instead of linthreshy but actuallynot realizing that they made a mistake, and are therefore generating wrong graphs (note that the older of the two issues has been opened for years and probably none of the reviewers who had a quick look at it noticed the typo from a quick review of the code). Which may be sufficiently correct for them in the sense that they can use them and publish them and put them in presentations and whatnot, but are still wrong. |
Let me give an example for "sufficiently correct for the user". Previously |
anntzer commentedJul 25, 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.
As you mentioned, plt.show(fig) did the correct thing even though it should not be called that way. My claim is that when an incorrect keyword argument is passed to SymLogScale, it's most likelynot doing the correct thing, as shown by the bug reports; code that did so is most likelyalready broken (except that it generates wrong output rather than throwing an exception, but the former is worse). |
This applies to any keyword argument:
|
Obviously it is possble to use a footgun and shoot in the air rather than in one's foot. Again I would guess more people are shooting in their foot with it rather than in the air. Let's see if any other devs want to chime in with their opinion? |
Maybe I missed it, so what is the drawback of doing the deprecation with a warning as usual? (A warning by itself should already be enough to keep one's feet healthy) |
I guess I think that the general idea that folks should not be allowed to inadvertently make incorrect plots is a fine line to make when deciding whether to check kwargs... But,... 1) I think this scale is terrible, and I'm not convinced 90% of the people who use it (including the folks who wrote the issues) really understand what it does. Ihonestly think we should deprecate the whole thing. I think if something has vanishingly few published examples (are there any in real journals? I'd never let someone put this in a paper I was reviewing or editing),and it is subject to misunderstandings like this is, we should really reconsider its value. |
mostly agree on 1) |
Mhh, let's deprecate Back on topic: Just deprecate passing unused kwargs to this, or any function or method you like and everyone is happy. (Use a one-minor-release-cycle if you think it's urgent - no opposition on that from my side) |
Given that there are hundreds of thousands of peer-reviewed articles that use |
@tacaswell You put in a similar API tightening in6357245 without a deprecation period. Would you support doing the same here (see discussion above)? I will defer to your judgment. |
We should all take it down a notch, we all want the same thing at the end of the day, just disagree a about what some details along the way are :) TL; DR : we should do a deprecation cycle here, I'll push some commits. @ImportanceOfBeingErnest is completely correct that this is an API change and should be documented and go through a deprecation cycle. @anntzer is also completely correct that I set precedent of making this sort of API change in a patch release (!). In my judgement@timhoffm and@anntzer are almost almost certainly correct that everyone we will catch with this changeintend to be passing a kwarg we do not ignore, however@ImportanceOfBeingErnest is also correct that there is a chance that there is some library layer which is abusing the current behavior (I could imaging having a dict of kwargs On balance, I think I made a mistake in6357245 , and although we have not (to my knowledge) had any complaints about it, we should try to do better about not breaking down stream and users without warning. Technically the same goes for changing the exception type, but I think we should let that one through as there is no reasonable way to do warnings on that (I guess MI of the two exceptions but then how do you tell when you are caught by the 'wrong one'?). As a side note, these scales have been in from pre 1.0 days (via2e8ce81 in 2007). Log scales in the negative direction make perfect sense (you make it positive, take the log, put the negative backoutside), log has a linear region around 1 ( |
While log scale became strict about extra kwargs in a patch release in6357245 (by@tacaswell), I now think that was a mistake. We do wantto get to all of the scales raising on unexpected kwargs so deprecatethem in 3.2 to raise in 3.3.
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 approve of this in it's current state (but I now have a commit at the end).
Uh oh!
There was an error while loading.Please reload this page.
No deprecation period, both because6357245 likewise added the exception
for LogScale without deprecation, and because this is by far most likely
to be catching bugs rather than real uses.
Closes#5619,closes#14878.
PR Summary
PR Checklist