Update plot_modifications.py #5116

tiapac · 2025-02-11T16:54:55Z

I noticed that making a contour plot does not allow me to use the kwarg "cmap" in the 'plot_args' dictionary.
If I add the "cmap" key work to 'plot_args' as in

s = yt.ProjectionPlot(ds, "x", ("gas","density"))	
s.annotate_contour(("gas","density"), plot_args = { "cmap":"jet"})```

returns

Traceback (most recent call last):
  File ".conda/envs/ytenv/lib/python3.12/site-packages/yt/visualization/plot_window.py", line 1285, in run_callbacks
    callback(cbw)
  File ".conda/envs/ytenv/lib/python3.12/site-packages/yt/visualization/plot_modifications.py", line 1032, in __call__
    cset = plot._axes.contour(xi, yi, zi, levels, **self.plot_args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".conda/envs/ytenv/lib/python3.12/site-packages/matplotlib/__init__.py", line 1473, in inner
    return func(
           ^^^^^
  File ".conda/envs/ytenv/lib/python3.12/site-packages/matplotlib/axes/_axes.py", line 6659, in contour
    contours = mcontour.QuadContourSet(self, *args, **kwargs)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".conda/envs/ytenv/lib/python3.12/site-packages/matplotlib/contour.py", line 801, in __init__
    raise ValueError('Either colors or cmap must be None')

ValueError: Either colors or cmap must be None

The cause is that by default yt partially generate said dictionary ( in yt/visualization/plot_modifications.py, line 931 ) and then add the user supplied options:

class ContourCallback(PlotCallback):
...
...
...
    def __init__(
        self,
        field: AnyFieldKey,
        levels: int = 5,
        *,
        factor: tuple[int, int] | int = 4,
        clim: tuple[float, float] | None = None,
        label: bool = False,
        take_log: bool | None = None,
        data_source: YTDataContainer | None = None,
        plot_args: dict[str, Any] | None = None,
        text_args: dict[str, Any] | None = None,
        ncont: int | None = None,  # deprecated
    ) -> None:
...
...
...
self.plot_args = {
    "colors": "black",
    "linestyles": "solid",
    **(plot_args or {}),
}

This generate a conflict in matplotlib because when "colors" is supplied, "cmap" is not accepted.

A solution might be to only add the default values if they are not supplied by the user in plot_args , as also suggested by @chrishavlin .

This is the content of the pull request.

welcome · 2025-02-11T16:54:57Z

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

chrishavlin · 2025-02-11T16:57:33Z

@tiapac did you mean to close this one?

tiapac · 2025-02-11T17:15:54Z

Yes, I modified through the online editor, so I was worried that the indentation was not correct. I also wanted to add a bit of context to the changes for future references. I tested it locally and it's ok, so I'll reopen it.
Sorry, I am still a bit clumsy with git outside my private repositories.

chrishavlin · 2025-02-11T17:32:13Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

chrishavlin · 2025-02-11T17:33:46Z

@tiapac I just triggered the pre-commit bot to fix the failing style check: locally you'll want to run git pull to make sure you pull those changes locally.

tiapac · 2025-02-11T19:06:55Z

The type check test gives an error here:

Run mypy yt
  mypy yt
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.10.16/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.10.16/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.16/x64
    Python[2](https://github.com/yt-project/yt/actions/runs/13268926486/job/37043480434?pr=5116#step:6:2)_ROOT_DIR: /opt/hostedtoolcache/Python/[3](https://github.com/yt-project/yt/actions/runs/13268926486/job/37043480434?pr=5116#step:6:3).10.16/x64
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.10.16/x6[4](https://github.com/yt-project/yt/actions/runs/13268926486/job/37043480434?pr=5116#step:6:4)
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.10.16/x64/lib
yt/visualization/plot_modifications.py: note: In member "__init__" of class "ContourCallback":
yt/visualization/plot_modifications.py:966: error: Unsupported right operand type for in ("dict[str, Any] | None")  [operator]
yt/visualization/plot_modifications.py:968: error: Unsupported right operand type for in ("dict[str, Any] | None")  [operator]
yt/utilities/performance_counters.py: note: In member "print_stats" of class "PerformanceCounters":
yt/utilities/performance_counters.py:73: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Found 2 errors in 1 file (checked [5](https://github.com/yt-project/yt/actions/runs/13268926486/job/37043480434?pr=5116#step:6:5)16 source files)
Error: Process completed with exit code 1.

I think it is because if plot_args defaults to None, thus the in operation fails in such case.
A solution might be to switch to

if "colors" not in self.plot_args and "cmap" not in self.plot_args: ...
instead of
if "colors" not in plot_args and "cmap" not in plot_args: ... ?

chrishavlin · 2025-02-11T19:47:51Z

A solution might be to switch to

if "colors" not in self.plot_args and "cmap" not in self.plot_args: ...

yup! that should fix it.

Changed ``plot_args`` to ``self.plot_args`` in the if/in check

chrishavlin

Apologies for the delay in getting back to this. But the change looks good to me. It'd great to add a test though -- I'm going to write one real quick right now, do you mind if I push to your branch?

chrishavlin · 2025-02-24T16:25:05Z

do you mind if I push to your branch?

I just went ahead and did it, hope you don't mind :) you'll want to git pull again to get the new changes locally.

Also just FYI, in the future it's best practice to submit pull requests from new branches and keep your main branch as a clean copy of yt's main branch. For example, git checkout -b fix_contour_plot_args would have worked well here. since it's a small PR it's not worth fixing at this stage (to change branches for a PR after submission you'd have to close this PR and then open a new one).

chrishavlin · 2025-02-24T16:54:22Z

oh, oops, forgot to add the new test file to the nose config. i'll fix that!

tiapac · 2025-02-25T13:46:24Z

Sure, I don't mind. Thanks for the help.

Also just FYI, in the future it's best practice to submit pull requests from new branches and keep your main branch as a clean copy of yt's main branch. For example, git checkout -b fix_contour_plot_args would have worked well here. since it's a small PR it's not worth fixing at this stage (to change branches for a PR after submission you'd have to close this PR and then open a new one).

Thanks for the suggestion! :)

neutrinoceros · 2025-02-25T17:57:22Z

yt/visualization/plot_modifications.py

+        self.plot_args = {**(plot_args or {})}
+        if "colors" not in self.plot_args and "cmap" not in self.plot_args:
+            self.plot_args["colors"] = "black"
+        if "linestyles" not in self.plot_args:
+            self.plot_args["linestyles"] = "solid"


how about this instead ?

Suggested change

self.plot_args = {**(plot_args or {})}

if "colors" not in self.plot_args and "cmap" not in self.plot_args:

self.plot_args["colors"] = "black"

if "linestyles" not in self.plot_args:

self.plot_args["linestyles"] = "solid"

self.plot_args = {

"linestyles": "solid",

**(plot_args or {})

}

if "cmap" not in plot_args:

self.plot_args.set_default("colors", "black")

Sorry for coming back at it after a while, I'm going thorugh particularly busy weeks. The change works for me, looks cleaner with set_default.
Should we do the same for linestyles?

Suggested change

self.plot_args = {**(plot_args or {})}

if "colors" not in self.plot_args and "cmap" not in self.plot_args:

self.plot_args["colors"] = "black"

if "linestyles" not in self.plot_args:

self.plot_args["linestyles"] = "solid"

self.plot_args = {**(plot_args or {})

}

if "cmap" not in plot_args:

self.plot_args.set_default("colors", "black")

if "linestyles" not in plot_args:

self.plot_args.set_default("linestyles", "solid")

No. linestyles is already being overriden correctly, let's keep the more verbose style only for when it's really needed.

Note that the if "linestyles" not in plot_args condition is entirely redundant in your suggestion, too.

neutrinoceros · 2025-02-25T17:59:43Z

to echo Chris' comment on branching:
please have a look at https://hynek.me/articles/pull-requests-branch/

tiapac · 2025-03-05T09:55:54Z

@neutrinoceros Thanks for the suggested reading, very helpful!

Update plot_modifications.py

4a84111

tiapac closed this Feb 11, 2025

tiapac reopened this Feb 11, 2025

chrishavlin added the viz: 2D label Feb 11, 2025

[pre-commit.ci] auto fixes from pre-commit.com hooks

8611736

for more information, see https://pre-commit.ci

chrishavlin added the bug label Feb 11, 2025

Update plot_modifications.py

c9add43

Changed ``plot_args`` to ``self.plot_args`` in the if/in check

chrishavlin self-requested a review February 19, 2025 16:10

chrishavlin reviewed Feb 24, 2025

View reviewed changes

test contour callback cmap, color kwargs

ff9b352

chrishavlin previously approved these changes Feb 24, 2025

View reviewed changes

add test_callbacks_pytestonly to nose ignore lists

a321d64

chrishavlin dismissed their stale review via a321d64 February 24, 2025 16:59

neutrinoceros reviewed Feb 25, 2025

View reviewed changes

Update plot_modifications.py #5116

Are you sure you want to change the base?

Update plot_modifications.py #5116

Uh oh!

Conversation

tiapac commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

welcome bot commented Feb 11, 2025

Uh oh!

chrishavlin commented Feb 11, 2025

Uh oh!

tiapac commented Feb 11, 2025

Uh oh!

chrishavlin commented Feb 11, 2025

Uh oh!

chrishavlin commented Feb 11, 2025

Uh oh!

tiapac commented Feb 11, 2025

Uh oh!

chrishavlin commented Feb 11, 2025

Uh oh!

chrishavlin left a comment

Choose a reason for hiding this comment

Uh oh!

chrishavlin commented Feb 24, 2025

Uh oh!

chrishavlin commented Feb 24, 2025

Uh oh!

tiapac commented Feb 25, 2025

Uh oh!

neutrinoceros Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

tiapac Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Feb 25, 2025

Uh oh!

tiapac commented Mar 5, 2025

Uh oh!

Uh oh!

tiapac commented Feb 11, 2025 •

edited

Loading