Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: annotating a sphere then setting font size in a plot results in a crash #4698

Closed
neutrinoceros opened this issue Oct 10, 2023 · 3 comments · Fixed by #4699
Closed

BUG: annotating a sphere then setting font size in a plot results in a crash #4698

neutrinoceros opened this issue Oct 10, 2023 · 3 comments · Fixed by #4699
Milestone

Comments

@neutrinoceros
Copy link
Member

neutrinoceros commented Oct 10, 2023

Bug report

Bug summary

Discovered by RevathiJambunathan. She further reported that this might be a regression in yt 4.2.x

Code for reproduction

import yt

ds = yt.load_sample("IsolatedGalaxy")
sl = yt.SlicePlot(ds, 0, ("gas", "density"))

sl.annotate_sphere(
    [0, 0, 0],
    radius=(0.2, "Mpc"),
    circle_args={"color": "white", "linewidth": 2},
)
sl.set_font_size(24) # everything works fine if this line is dropped *or* moved before sl.annotate_sphere(...)

sl.save("/tmp/")

Actual outcome

/Users/robcleme/dev/yt-project/yt/yt/loaders.py:1670: UserWarning: This dataset appears to be of type EnzoDataset, but the following requirements are currently missing: libconf
Please verify your installation.
  return load(loadable_path, **load_kwargs)
Parsing Hierarchy : 100%|███████████████████████████████████████████████████████████████████████████████████| 173/173 [00:00<00:00, 12522.47it/s]
Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_window.py", line 1285, in run_callbacks
    callback(cbw)
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_modifications.py", line 2041, in __call__
    self.radius = self.radius.to(units)
                  ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.12/site-packages/unyt/array.py", line 947, in to
    return self.in_units(units, equivalence=equivalence, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.12/site-packages/unyt/array.py", line 874, in in_units
    (conversion_factor, offset) = self.units.get_conversion_factor(
                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.12/site-packages/unyt/unit_object.py", line 694, in get_conversion_factor
    return _get_conversion_factor(self, other_units, dtype)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/robcleme/.pyenv/versions/yt-dev/lib/python3.12/site-packages/unyt/unit_object.py", line 939, in _get_conversion_factor
    raise UnitConversionError(
unyt.exceptions.UnitConversionError: Cannot convert between '1/code_length' (dim '1/(length)') and 'code_length' (dim '(length)').

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t.py", line 14, in <module>
    sl.save("/tmp/")
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/_commons.py", line 162, in newfunc
    self._setup_plots()
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_window.py", line 1226, in _setup_plots
    self.run_callbacks()
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_window.py", line 1289, in run_callbacks
    raise YTPlotCallbackError(callback._type_name) from e
yt.utilities.exceptions.YTPlotCallbackError: annotate_sphere callback failed

Expected outcome

No crash

Version Information

  • Operating System: macOS
  • Python Version: 3.11.4
  • yt version: 4.3.0
  • Other Libraries (if applicable):
@chrishavlin
Copy link
Contributor

Looks like this is because the callback overwrites the radius attribute with a scaled version, which changes the units from length to either dimensionless value or 1/length. So when it runs back through the callback (cause calling set_font_size invalidates the plot), the radius attribute will be the already-scaled value with units that can't convert to dimensions of length.

https://github.com/yt-project/yt/blob/14c49c4fa832fb7f04485a16134d9adc2916508d/yt/visualization/plot_modifications.py#L2033C1-L2050C59

        if is_sequence(self.radius):
            self.radius = plot.data.ds.quan(self.radius[0], self.radius[1])
            self.radius = np.float64(self.radius.in_units(plot.xlim[0].units))
        if isinstance(self.radius, YTQuantity):
            if isinstance(self.center, YTArray):
                units = self.center.units
            else:
                units = "code_length"
            self.radius = self.radius.to(units)


        # This assures the radius has the appropriate size in
        # the different coordinate systems, since one cannot simply
        # apply a different transform for a length in the same way
        # you can for a coordinate.
        if self.coord_system == "data" or self.coord_system == "plot":
            self.radius = self.radius * self._pixel_scale(plot)[0]
        else:
            self.radius /= (plot.xlim[1] - plot.xlim[0]).v

simplest fix would be to not overwrite self.radius. but could also add a self._scaled_radius attribute to avoid re-calculating if it's already been through once? I'll keep playing around and push something up shortly.

@yut23
Copy link
Member

yut23 commented Oct 10, 2023

I did some searching and found one more callback with a similar issue: calling annotate_arrow() then another function that runs the callback changes the arrow size.

def __call__(self, plot):
x, y = self._sanitize_coord_system(
plot, self.pos, coord_system=self.coord_system
)
xx0, xx1, yy0, yy1 = self._plot_bounds(plot)
# normalize all of the kwarg lengths to the plot size
plot_diag = ((yy1 - yy0) ** 2 + (xx1 - xx0) ** 2) ** (0.5)
self.length *= plot_diag
self.width *= plot_diag
self.head_width *= plot_diag
if self.head_length is not None:

@chrishavlin
Copy link
Contributor

Thanks @yut23 ! I was curious if there'd be any others that failed when invalidated and run again, looks like the arrow callback fails for the same reason (storing calculations in class attributes). I'll add the fix to my current PR

@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants