Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix specifying number of levels with log contour#27576
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
Uh oh!
There was an error while loading.Please reload this page.
e0abbc2
toa0c5e26
Comparea0c5e26
tof495d8d
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thanks - as well as applying your suggestions, I also factored out the logic to set the locator if it's not already set. This allows the long docstrings to be split up, and makes it explicit in |
Uh oh!
There was an error while loading.Please reload this page.
if args: | ||
# Set if levels manually provided | ||
levels_arg = args[0] |
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.
This could still be made clearer / the current code is still convoluted (but possibly better as a followup PR to not make too many changes and complicate review)
args
is not the originalargs
but stripped from leading[X, Y], Z
. Effectively it can only be empty or contain a single element that containslevels
passed positionally (which is supported, but a bit cumbersome to write out as a signature because the optional[X, Y]
require to absorb all positional parameters in*args
and we therefore have to reunite a possible positional level from args with the possible kwarg level.
The current implementation also has the surprising side effect that you can docontour(Z, levels1, levels=levels2)
and the positionallevels1
is sliently ignored.
The only use ofargs
here is to pass on a possible positionallevel
. We can push the logic out of the function and unite the positional and kwarg paths oflevels
in the caller_contour_args
- where it belongs logically.
I think eventually we should also makelevels
kw-only.contour(Z, 5)
orcontour(X, Y, Z, [-1, 0, 1])
are not very readable anyway.
Uh oh!
There was an error while loading.Please reload this page.
I've milestoned this as 3.11 because it changes longstanding behaviour (even if it is a bugfix) |
7fd4c67
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
When plotting contours with a log norm, passing an integer value to the
levels
argument to cap the maximum number of contour levels now works as intended. Partially addresses#19856 and replaces#25149.I've maually checked that the added test fails before this fix. Testing with the following script:
Gives on current
main
:and with this PR:
PR checklist