Skip to content
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 passing formatting parameters positionally to stem() #21126

Merged
merged 1 commit into from Sep 24, 2021

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Sep 19, 2021

PR Summary

This will allow to simplify the implementation because, currently,
stem(*args, linefmt=None, ...) still tries to resolve excess
positionally passed args, to linefmt and following parameters, which
is quite a bit of logic (see # fallback to positional argument comments
in the implementation).

OTOH, since we have 3 formats, passing them positionally is already
difficult from a usability/readability perspective, because they can
easily be mixed up.

@timhoffm timhoffm added this to the v3.5.0 milestone Sep 19, 2021
@timhoffm timhoffm changed the title Make stem formatting parameters keyword only. Deprecate passing formatting parameters positionally to stem() Sep 19, 2021
@timhoffm timhoffm force-pushed the stem-kwonly branch 2 times, most recently from e68a2bf to 07cbdfc Compare September 19, 2021 13:27
@anntzer
Copy link
Contributor

anntzer commented Sep 19, 2021

I hardly ever use stem() myself, so I can't claim to have much experience, but my guess is that stem(x, y, "<fmt>") is likely considered quite practical by whoever uses it. Perhaps the further fmts can be made kwonly, but making linefmt kwonly seems(??) bad for usability.
Moreover, the args parsing code can be much simplified; see #21127.

@timhoffm timhoffm marked this pull request as draft September 19, 2021 18:50
@timhoffm
Copy link
Member Author

Will come back after #21127 and decide if we should deprecate nevertheless.

@timhoffm
Copy link
Member Author

but my guess is that stem(x, y, "<fmt>") is likely considered quite practical by whoever uses it.

It's rather a foot cannon. One can be tempted to define the marker here (which I would consider a more common change than the line). But that does not work. This format string only applies to the stem lines and marker information is silently ignored.

Actually even one of our tests is trying to do that:

ax.stem(xs, ys, "*-")

The '*' here is ignored.

Also, the stem3d signature already has linefmt as kw-only.

On a side note, the make_keyword_only decorator cannot be used with the *args case.

@_api.make_keyword_only("3.5", 'linefmt')
def stem(self, *args, linefmt=None, ...)

raises

~/dev/matplotlib/lib/matplotlib/_api/deprecation.py in make_keyword_only(since, name, func)
    436     assert (name in signature.parameters
    437             and signature.parameters[name].kind == POK), (
--> 438         f"Matplotlib internal error: {name!r} must be a positional-or-keyword "
    439         f"parameter for {func.__name__}()")
    440     names = [*signature.parameters]

AssertionError: Matplotlib internal error: 'linefmt' must be a positional-or-keyword parameter for stem()

@timhoffm timhoffm force-pushed the stem-kwonly branch 2 times, most recently from e6bf835 to 1c66f56 Compare September 22, 2021 23:46
@anntzer
Copy link
Contributor

anntzer commented Sep 23, 2021

Regarding your side point, that's because linefmt is already kwonly in the example you give.

@timhoffm timhoffm marked this pull request as ready for review September 23, 2021 06:53


def test_stem_dates():
fig, ax = plt.subplots(1, 1)
xs = [dateutil.parser.parse("2013-9-28 11:00:00"),
dateutil.parser.parse("2013-9-28 12:00:00")]
ys = [100, 200]
ax.stem(xs, ys, "*-")
ax.stem(xs, ys, linefmt="*-")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the * is still ignored here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Actually, the linefmt doesn't do anything at all here. Solid is already the default, so we can just remove the parameter.

This will allow to simplify the implementation because, currently,
`stem(*args, linefmt=None, ...)` still tries to resolve excess
positionally passed args, to *linefmt* and following parameters, which
is quite a bit of logic.

OTOH, since we have 3 formats, passing them positionally is already
difficult from a usability/readability perspective, because they can
easily be mixed up.
@story645
Copy link
Member

story645 commented Sep 23, 2021

@timhoffm do you still want this to be merged for 3.5?

@timhoffm
Copy link
Member Author

The deprecation reduces the risk of unnoticed misuse, so the sooner this gets in the better. OTOH it's not very important. It should not hold up a release or put somebody under pressure to react to it for 3.5.

@QuLogic QuLogic merged commit d4b9d63 into matplotlib:master Sep 24, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 24, 2021
QuLogic added a commit that referenced this pull request Sep 25, 2021
…126-on-v3.5.x

Backport PR #21126 on branch v3.5.x (Deprecate passing formatting parameters positionally to stem())
@timhoffm timhoffm deleted the stem-kwonly branch September 25, 2021 03:26
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 12, 2021
Deprecate passing formatting parameters positionally to stem()
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
Deprecate passing formatting parameters positionally to stem()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants