Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 30, 2025
Merged

Fix specifying number of levels with log contour #27576

merged 8 commits into from
Apr 30, 2025

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Dec 28, 2023 •
edited
Loading

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:

import matplotlib.pyplot as plt
from matplotlib.colors import LogNorm
import numpy as np

x, y = np.mgrid[1:10:0.1, 1:10:0.1]
data = np.abs(np.sin(x)*np.exp(y))

for n_levels in range(2, 20):
    fig, ax = plt.subplots()
    im = ax.contourf(x, y, data, norm=LogNorm(), levels=n_levels)
    cbar = fig.colorbar(im, ax=ax)
    n_ticks = len(cbar.ax.get_yticklabels())
    print(f"Requested max {n_levels + 1}, got {n_ticks - 1}")

Gives on current main:

Requested max 3, got 7
Requested max 4, got 7
Requested max 5, got 7
Requested max 6, got 7
Requested max 7, got 7
Requested max 8, got 7
Requested max 9, got 7
Requested max 10, got 7
Requested max 11, got 7
Requested max 12, got 7
Requested max 13, got 7
Requested max 14, got 7
Requested max 15, got 7
Requested max 16, got 7
Requested max 17, got 7
Requested max 18, got 7
Requested max 19, got 7
Requested max 20, got 7

and with this PR:

Requested max 3, got 3
Requested max 4, got 4
Requested max 5, got 4
Requested max 6, got 4
Requested max 7, got 7
Requested max 8, got 7
Requested max 9, got 7
Requested max 10, got 7
Requested max 11, got 7
Requested max 12, got 7
Requested max 13, got 7
Requested max 14, got 7
Requested max 15, got 7
Requested max 16, got 7
Requested max 17, got 7
Requested max 18, got 7
Requested max 19, got 7
Requested max 20, got 7

PR checklist

@dstansby dstansby mentioned this pull request Dec 28, 2023
Closed
@dstansby dstansby marked this pull request as ready for review December 28, 2023 21:13
ksunden
ksunden reviewed Jan 9, 2024
View reviewed changes
@ebarcelosf ebarcelosf mentioned this pull request Nov 13, 2024
Open
5 tasks
@dstansby dstansby force-pushed the log-contour-levels branch from f495d8d to 9cd6171 Compare March 24, 2025 21:49
@dstansby dstansby requested a review from ksunden March 27, 2025 09:31
@dstansby dstansby force-pushed the log-contour-levels branch from e908660 to 042e55a Compare April 8, 2025 08:51
timhoffm
timhoffm reviewed Apr 8, 2025
View reviewed changes
timhoffm
timhoffm reviewed Apr 8, 2025
View reviewed changes
@dstansby
Copy link
Member Author

dstansby commented Apr 9, 2025

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 _process_contour_level_args where the default locator is being set, instead of hiding it in _autolev.

timhoffm
timhoffm approved these changes Apr 9, 2025
View reviewed changes
lib/matplotlib/contour.py
Comment on lines 1028 to 1029
if args:
# Set if levels manually provided
levels_arg = args[0]
Copy link
Member

@timhoffm timhoffm Apr 9, 2025 •
edited
Loading

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 original args but stripped from leading [X, Y], Z. Effectively it can only be empty or contain a single element that contains levels 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 do contour(Z, levels1, levels=levels2) and the positional levels1 is sliently ignored.

The only use of args here is to pass on a possible positional level. We can push the logic out of the function and unite the positional and kwarg paths of levels in the caller _contour_args - where it belongs logically.

I think eventually we should also make levels kw-only. contour(Z, 5) or contour(X, Y, Z, [-1, 0, 1]) are not very readable anyway.

QuLogic
QuLogic reviewed Apr 16, 2025
View reviewed changes
@dstansby dstansby force-pushed the log-contour-levels branch from cb2a823 to 8c0775e Compare April 30, 2025 09:28
@dstansby dstansby requested a review from QuLogic April 30, 2025 09:28
@dstansby dstansby added this to the v3.11.0 milestone Apr 30, 2025
@dstansby
Copy link
Member Author

I've milestoned this as 3.11 because it changes longstanding behaviour (even if it is a bugfix)

@dstansby dstansby merged commit 7fd4c67 into matplotlib:main Apr 30, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

@ksunden ksunden Awaiting requested review from ksunden

Assignees
No one assigned
Labels
topic: contour
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@dstansby @QuLogic @ksunden @timhoffm