add the ability to set the transfer function label fontsize #4481

zingale · 2023-06-07T19:55:21Z

PR Summary

PR Checklist

New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

matthewturk

I like it!

zingale · 2023-06-07T19:56:30Z

Here's the old (now default) behavior:

and here's a render with label_fontsize=20:

zingale · 2023-06-07T19:57:00Z

Here's the script for the above:

import yt
from yt.visualization.volume_rendering.api import Scene, create_volume_source

# Load the dataset.
ds = yt.load_sample("IsolatedGalaxy")
vol = create_volume_source(ds.all_data(), field=("gas", "density"))
sc = Scene()
sc.add_source(vol)
cam = sc.add_camera(ds, lens_type="perspective")
cam.resolution = (1920, 1280)

# You may need to adjust the alpha values to get a rendering with good contrast
# For annotate_domain, the fourth color value is alpha.


# Draw a coordinate axes triad
sc.annotate_axes(alpha=0.01, thickness=1)
sc.save_annotated(f"{ds}_vr_coords.png", sigma_clip=4)

zingale · 2023-06-07T20:08:51Z

pre-commit.ci autofix

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

neutrinoceros · 2023-06-07T20:21:33Z

yt/visualization/volume_rendering/scene.py

@@ -364,6 +364,7 @@ def save_annotated(
        self,
        fname: Optional[str] = None,
        label_fmt: Optional[str] = None,
+        label_fontsize: int = 10,


We can't add allowed-positional arguments to public functions in arbitrary places in the signature because it might break existing user code. Since everything here is currently allowed-positional, any new argument must be appended at the end. And because allowed-positional are a nightmare to deprecate if they ever need to be, let's make it keyword-only.

I don't really use type hints, so I messed this up. Does this mean I should add Optional[]?

No, your type hint is perfect (unless floats are also valid, in which case the annotation should reflect it). For reference, Optional[T] means "can be T or None". It's not needed here because the default value already has the proper type.

What I'm advocating for here is making new arguments keyword-only.

…nnotate_tick_fontsize

neutrinoceros · 2023-06-08T08:45:24Z

yt/visualization/volume_rendering/transfer_functions.py

+        self,
+        resolution,
+        log_scale,
+        ax,
+        *,
+        label=None,
+        label_fmt=None,
+        label_fontsize=10,
+        size=10,


label and label_fmt are currently allowed-positional, so making them suddenly keyword-only is a breaking change

Suggested change

self,

resolution,

log_scale,

ax,

*,

label=None,

label_fmt=None,

label_fontsize=10,

size=10,

self,

resolution,

log_scale,

ax,

label=None,

label_fmt=None,

*,

label_fontsize=10,

size=10,

neutrinoceros · 2023-06-08T09:17:47Z

yt/visualization/volume_rendering/scene.py

@@ -369,6 +369,8 @@ def save_annotated(
        sigma_clip: Optional[float] = None,
        render: bool = True,
        tf_rect: Optional[List[float]] = None,
+        *,
+        label_fontsize: int = 10,


given that it ends up being passed to matplotlib.axes.Axes.tick_params, the proper type is actually Union[float, str] ('large' also works) (see https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.tick_params.html)

(Union needs to be imported as from typing import Union)

neutrinoceros · 2023-06-08T09:19:42Z

yt/visualization/volume_rendering/scene.py

@@ -399,6 +401,9 @@ def save_annotated(
        label_fmt : str, optional
           A format specifier (e.g., label_fmt="%.2g") to use in formatting
           the data values that label the transfer function colorbar.
+        label_fontsize : int, optional


the docstring should mention what the default value is.

... and now it should be synced with the type annotation.
It'd be worth linking to https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.tick_params.html#matplotlib.axes.Axes.tick_params
too, so users get first hand documentation for what strings mean in this context.
But maybe we could just type this as float and hide this implementation detail.

(sorry that it's taking so many iterations, I promise we'll converge after this one)

updated. I didn't put the URL in, but I gave some examples. I don't see any other docstrings that embed URLs.

I know a couple docstring where urls to matplotlib doc would be very useful, but here it's not crucial. Thanks !

zingale · 2023-06-08T17:14:33Z

pre-commit.ci autofix

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

…nnotate_tick_fontsize

neutrinoceros · 2023-06-09T14:02:47Z

yt/visualization/volume_rendering/transfer_functions.py

@@ -686,7 +694,7 @@ def y_format(y, pos):
        ax.set_xlim(0.0, max_alpha)
        ax.get_xaxis().set_ticks([])
        ax.set_ylim(visible[0].item(), visible[-1].item())
-        ax.tick_params(axis="y", colors="white", size=10)
+        ax.tick_params(axis="y", colors="white", labelsize=label_fontsize)


Took me long enough (sorry again) but I just realised that you're exposing the tick label size, not the size of the colorbar label itself (in fact, it's already exposed as the size parameter). I think the parameter would be better named tick_label_size.

I also see now that the actual colorbar label size is automatically scaled according to the resolution, so wouldn't it make sense to do the same thing with tick labels so their size, relative to the colorbar label, would be resolution-independent ?
There's still value in exposing both parameters independently as you're doing here, so we can keep the bulk of your patch as is, but we'd need remove references to special strings from matplotlib.

all of the label parameters (this would make 3) are for the tick labels. So if you change this one, all 3 should be changed.

ah, I see. Let's keep the name then. What about auto-scaling ?

matthewturk · 2023-06-09T14:09:40Z

I think we can probably accept the patch as-is and supply additional patches if truly necessary. This is a useful piece of functionality and I don't want us to get too strict.

neutrinoceros · 2023-06-09T14:19:56Z

I'd be happy to make the change I'm proposing as a follow up PR if it wasn't going to break what's introduced here. I can however propose to make the patch myself within this PR

matthewturk · 2023-06-09T14:55:10Z

I think we should try to keep the number of iterations as low as possible, which I know you also agree with -- and to that end, compromise on some things that will have minimal impact on the potential or actual end users. (It's hard because I too don't always know exactly the right end point or where things will go when I start reviewing!) I don't think we need to get into it, because I think we're probably in agreement, and I just wanted to note that I think that balance is something that in this particular case we should continue to pay attention to.

zingale · 2023-06-09T16:08:18Z

I'd prefer to deal with the autoscaling in a different PR. I'm indifferent to it.

neutrinoceros · 2023-06-09T19:19:44Z

Ok, I'm willing to do the follow up PR but I might not be able to guarantee 100% stability with respect to this one doing so (I expect you won't need to use your new, still valuable, keyword argument after that). Does this sound acceptable to you ?

zingale · 2023-06-09T19:48:28Z

oh, I think I misunderstood you. It is important to be able to set the size of the variable name and the size of the labels (what you call ticks) independently.

neutrinoceros · 2023-06-09T19:55:55Z

Agreed, that's why I still think your patch has value in itself. But getting legible label size by default would also help, wouldn't it ?

matthewturk · 2023-08-22T17:13:23Z

This looks pretty good to me as is, and I'm not sure I understand the disagreement.

neutrinoceros · 2023-08-28T16:36:15Z

I didn't mean to block this. I'm willing to start the conversation afresh.

neutrinoceros · 2023-09-08T17:22:57Z

I apologies for how long it took me to circle back here. It's become hard for me to return to the mindset in which I wrote my last review, and I don't want to block this any longer.
Resolving the merge conflict should be sufficient to get this in.

neutrinoceros · 2023-09-08T17:24:35Z

pre-commit.ci autofix

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

neutrinoceros · 2023-09-08T17:52:22Z

@yt-fido test this please

neutrinoceros · 2023-09-08T19:19:06Z

@yt-fido test this please

add the ability to set the transfer function label fontsize

bd82278

zingale added enhancement Making something better viz: volume rendering labels Jun 7, 2023

matthewturk previously approved these changes Jun 7, 2023

View reviewed changes

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

4a7bf3f

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

pre-commit-ci bot dismissed matthewturk’s stale review via 4a7bf3f June 7, 2023 20:09

neutrinoceros requested changes Jun 7, 2023

View reviewed changes

zingale added 2 commits June 7, 2023 17:23

positional obly

70b84f6

Merge branch 'annotate_tick_fontsize' of github.com:zingale/yt into a…

27ae4a6

…nnotate_tick_fontsize

neutrinoceros reviewed Jun 8, 2023

View reviewed changes

address more review comments

28d95fe

pre-commit-ci bot and others added 3 commits June 8, 2023 17:15

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

54c1046

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

update docstring

7625ae5

Merge branch 'annotate_tick_fontsize' of github.com:zingale/yt into a…

c72d865

…nnotate_tick_fontsize

neutrinoceros reviewed Jun 9, 2023

View reviewed changes

Merge branch 'main' into annotate_tick_fontsize

082f569

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

5252be9

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

neutrinoceros added this to the 4.3.0 milestone Sep 8, 2023

neutrinoceros approved these changes Sep 8, 2023

View reviewed changes

neutrinoceros enabled auto-merge (squash) September 8, 2023 17:28

neutrinoceros merged commit f4ff824 into yt-project:main Sep 8, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add the ability to set the transfer function label fontsize #4481

add the ability to set the transfer function label fontsize #4481

zingale commented Jun 7, 2023

matthewturk left a comment

zingale commented Jun 7, 2023

zingale commented Jun 7, 2023

zingale commented Jun 7, 2023

neutrinoceros Jun 7, 2023

zingale Jun 7, 2023

neutrinoceros Jun 7, 2023

zingale Jun 7, 2023

neutrinoceros Jun 8, 2023

zingale Jun 8, 2023

neutrinoceros Jun 8, 2023 •

edited

zingale Jun 8, 2023

neutrinoceros Jun 8, 2023

zingale Jun 8, 2023

neutrinoceros Jun 9, 2023

zingale Jun 9, 2023

neutrinoceros Jun 9, 2023

zingale commented Jun 8, 2023

neutrinoceros Jun 9, 2023

zingale Jun 9, 2023

neutrinoceros Jun 9, 2023

matthewturk commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

matthewturk commented Jun 9, 2023

zingale commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

zingale commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

matthewturk commented Aug 22, 2023

neutrinoceros commented Aug 28, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

add the ability to set the transfer function label fontsize #4481

add the ability to set the transfer function label fontsize #4481

Conversation

zingale commented Jun 7, 2023

PR Summary

PR Checklist

matthewturk left a comment

Choose a reason for hiding this comment

zingale commented Jun 7, 2023

zingale commented Jun 7, 2023

zingale commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neutrinoceros Jun 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zingale commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewturk commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

matthewturk commented Jun 9, 2023

zingale commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

zingale commented Jun 9, 2023

neutrinoceros commented Jun 9, 2023

matthewturk commented Aug 22, 2023

neutrinoceros commented Aug 28, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros commented Sep 8, 2023

neutrinoceros Jun 8, 2023 •

edited