Support only positional args in contour. Error if no positional argument. #24749

chahak13 · 2022-12-16T16:04:15Z

PR Summary

Closes #24743 (Extended from #24747)

Check if kwargs are passed for [X, Y], Z and if not, check that args has at least one element before accessing it.

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).
New plotting related features are documented with examples.

Release Notes

New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

chahak13 · 2022-12-16T16:05:01Z

@oscargus can you take a look at this? I've added some logic to support kwargs too. Let me know if you want to add these changes to the other PR and close this. Thanks!

lib/matplotlib/tests/test_contour.py

oscargus

Subject to green etc.

(And possibly the "black-like" formatting... ;-))

(One may consider reordering the logic a bit, but that is only my micro-optimization mindset talking. Like

if "Z" in kwargs:
    if "X" ... and ...
    elif "X" ... ^ ...
    else:
elif 0 < nargs...

but it may be that other people prefer to have it more understandable...)

This is much better than mine!

lib/matplotlib/tests/test_contour.py

lib/matplotlib/contour.py

tacaswell

I think the type of the raised error needs to be changed.

Anyone can dismiss this review.

chahak13 · 2022-12-17T04:29:58Z

Updated, @tacaswell.

jklymak · 2022-12-17T05:08:48Z

lib/matplotlib/contour.py

+                "Both or neither 'X' and 'Y' may be passed as kwargs. "
+                "Passing only one of the two is not allowed."


Suggested change

"Both or neither 'X' and 'Y' may be passed as kwargs. "

"Passing only one of the two is not allowed."

"Passing only one of 'X' or 'Y' is not allowed."

Change made

lib/matplotlib/tests/test_contour.py

jklymak · 2022-12-17T16:46:53Z

lib/matplotlib/contour.py

+            )
+        elif (("X" in kwargs) ^ ("Y" in kwargs)) and "Z" in kwargs:
+            raise TypeError(
+                "Both or neither 'X' and 'Y' may be passed as kwargs. "


Suggested change

"Both or neither 'X' and 'Y' may be passed as kwargs. "

You don't need to say both, and this phrase is extremely awkward.

timhoffm · 2022-12-17T17:06:57Z

Sorry for being late to the party. Are we sure we want to do this? Having positional-only parameters is not the end of the world. Note that we have a lot of similar functions, in which you cannot pass the coordinate parameters by keyword. I guess this holds for almost all functions that use *args for signature overloading. This keyword dance adds complexity. That is still moderate here, but if we go down this route users will expect this to work for all functions. I have serious concerns that we want to fiddle this into already more complex functions like tricontour.

I propose to instead only document that these parameters are keyword-only.

Alternatively, maybe it's worth investigating whether this can be generalized into a decorator to keep the complexity out of the functions and make it applicable throughout the code base. @anntzer you come to my mind when it comes to such types of infrastructure code 😄 .

anntzer · 2022-12-17T17:24:02Z

See #11526 (comment) or #22303. I haven't checked whether the situation is different for contour (perhaps not having U, V makes things easier) but at first glance keeping parameters positional-only may be a good idea.

If you want something more infrastruture-y, perhaps something along the lines of #7966 (comment) can be made to work (perhaps with a variant of select_matching_signature that displays all possible signatures in the exception thrown when an invalid call is made).

chahak13 · 2022-12-18T04:28:44Z

Based on the discussion, I agree that having it just as positional and erroring out if the required arguments are not found would be better. I can change it to something like my original fix here #24747 (comment)

Does that sound better? I also really like the idea of moving the parsing into a separate function as @anntzer did. I can see merits for first fixing this using just the positional arguments fix and then discussing about making a bigger change on the infra level but I'll defer that decision :)

tacaswell · 2022-12-18T18:53:42Z

Based in this discussion I think we should push this to 3.8.

tacaswell

Hold for 3.8, anyone can dismiss this once 3.7.x is branched.

lib/matplotlib/tests/test_contour.py

chahak13 · 2022-12-22T05:04:13Z

I'm not sure why code coverage is failing. I've not added any untested lines. The report is about other files, it seems.

QuLogic · 2022-12-22T05:49:50Z

Because for some reason no CI reported at all.

chahak13 · 2022-12-22T05:57:22Z

Should I change anything then?

QuLogic · 2022-12-22T06:04:58Z

Probably a rebase would fix it?

chahak13 · 2022-12-22T07:17:37Z

That seems to have at least triggered all the tests. Let's see. Thanks!

3.7 has been branched

ksunden · 2023-01-27T00:08:14Z

Conflict caused by #24870 which changed the inside of the if/elif structure (and added a separate test, which also conflicts, but is more trivial as both should be kept)

oscargus reviewed Dec 16, 2022

View reviewed changes

lib/matplotlib/tests/test_contour.py Outdated Show resolved Hide resolved

oscargus approved these changes Dec 16, 2022

View reviewed changes

oscargus reviewed Dec 16, 2022

View reviewed changes

lib/matplotlib/tests/test_contour.py Outdated Show resolved Hide resolved

oscargus added API: argument checking topic: contour PR: bugfix Pull requests that fix identified bugs labels Dec 16, 2022

oscargus added this to the v3.7.0 milestone Dec 16, 2022

oscargus added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 16, 2022

tacaswell reviewed Dec 16, 2022

View reviewed changes

lib/matplotlib/contour.py Outdated Show resolved Hide resolved

tacaswell previously requested changes Dec 16, 2022

View reviewed changes

chahak13 force-pushed the 24743_contour_kwargs branch 2 times, most recently from 805e4f0 to cba8e7d Compare December 17, 2022 04:57

jklymak reviewed Dec 17, 2022

View reviewed changes

jklymak requested a review from tacaswell December 17, 2022 05:08

QuLogic reviewed Dec 17, 2022

View reviewed changes

lib/matplotlib/tests/test_contour.py Outdated Show resolved Hide resolved

jklymak reviewed Dec 17, 2022

View reviewed changes

tacaswell previously requested changes Dec 18, 2022

View reviewed changes

tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 18, 2022

chahak13 changed the title ~~Support kwargs calls in contour~~ Support only positional args in contour. Error if no positional argument. Dec 19, 2022

timhoffm requested a review from oscargus December 19, 2022 20:57

timhoffm approved these changes Dec 19, 2022

View reviewed changes

oscargus removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 20, 2022

oscargus reviewed Dec 20, 2022

View reviewed changes

lib/matplotlib/tests/test_contour.py Outdated Show resolved Hide resolved

chahak13 force-pushed the 24743_contour_kwargs branch from d8e791d to 66a8722 Compare December 20, 2022 10:04

Support only positional args for data in contour

756eb1e

chahak13 force-pushed the 24743_contour_kwargs branch from 66a8722 to 756eb1e Compare December 22, 2022 07:14

github-actions bot added the status: needs rebase label Jan 4, 2023

Merge branch 'main' into 24743_contour_kwargs

d5baa19

github-actions bot removed the status: needs rebase label Jan 27, 2023

ksunden approved these changes Jan 27, 2023

View reviewed changes

ksunden merged commit 6ee6793 into matplotlib:main Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support only positional args in contour. Error if no positional argument. #24749

Support only positional args in contour. Error if no positional argument. #24749

chahak13 commented Dec 16, 2022

chahak13 commented Dec 16, 2022

oscargus left a comment

tacaswell left a comment

chahak13 commented Dec 17, 2022

jklymak Dec 17, 2022

chahak13 Dec 17, 2022 •

edited

Loading

jklymak Dec 17, 2022

timhoffm commented Dec 17, 2022 •

edited

Loading

anntzer commented Dec 17, 2022 •

edited

Loading

chahak13 commented Dec 18, 2022

tacaswell commented Dec 18, 2022

tacaswell left a comment

chahak13 commented Dec 22, 2022

QuLogic commented Dec 22, 2022

chahak13 commented Dec 22, 2022

QuLogic commented Dec 22, 2022

chahak13 commented Dec 22, 2022

ksunden commented Jan 27, 2023

		"Both or neither 'X' and 'Y' may be passed as kwargs. "
		"Passing only one of the two is not allowed."

	"Both or neither 'X' and 'Y' may be passed as kwargs. "
	"Passing only one of the two is not allowed."
	"Passing only one of 'X' or 'Y' is not allowed."

Support only positional args in contour. Error if no positional argument. #24749

Support only positional args in contour. Error if no positional argument. #24749

Conversation

chahak13 commented Dec 16, 2022

PR Summary

PR Checklist

chahak13 commented Dec 16, 2022

oscargus left a comment

Choose a reason for hiding this comment

tacaswell left a comment

Choose a reason for hiding this comment

chahak13 commented Dec 17, 2022

jklymak Dec 17, 2022

Choose a reason for hiding this comment

chahak13 Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

jklymak Dec 17, 2022

Choose a reason for hiding this comment

timhoffm commented Dec 17, 2022 • edited Loading

anntzer commented Dec 17, 2022 • edited Loading

chahak13 commented Dec 18, 2022

tacaswell commented Dec 18, 2022

tacaswell left a comment

Choose a reason for hiding this comment

chahak13 commented Dec 22, 2022

QuLogic commented Dec 22, 2022

chahak13 commented Dec 22, 2022

QuLogic commented Dec 22, 2022

chahak13 commented Dec 22, 2022

ksunden commented Jan 27, 2023

chahak13 Dec 17, 2022 •

edited

Loading

timhoffm commented Dec 17, 2022 •

edited

Loading

anntzer commented Dec 17, 2022 •

edited

Loading