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

Plot callbacks do not validate input parameters on call #3945

Closed
neutrinoceros opened this issue May 27, 2022 · 1 comment · Fixed by #3957
Closed

Plot callbacks do not validate input parameters on call #3945

neutrinoceros opened this issue May 27, 2022 · 1 comment · Fixed by #3957
Labels
enhancement Making something better UX user-experience

Comments

@neutrinoceros
Copy link
Member

neutrinoceros commented May 27, 2022

Bug report

Bug summary
Plot classes have annotate_<x> methods, which are internally referred to as "plot callbacks", and are implemented as classes, inheriting the bas class PlotCallback.
Nominally, these classes have an __init__ and a __call__ method. By design, __call__ isn't actually used before the plot is actually rendered (this is the very essence of a "callback"). However, I noticed that it's also the case for __init__ methods. This makes is possible to call annotate_<x> methods with completely invalid arguments without erroring; any error will only show up at render time, which can be quite confusing

Code for reproduction

Here's an example session to demonstrate the problem

>>> import yt
>>> from yt.testing import fake_amr_ds

>>> ds = fake_amr_ds(fields=["density"], units=["g/cm**3"])
yt : [INFO     ] 2022-05-27 11:41:21,888 Parameters: current_time              = 0.0
yt : [INFO     ] 2022-05-27 11:41:21,888 Parameters: domain_dimensions         = [32 32 32]
yt : [INFO     ] 2022-05-27 11:41:21,888 Parameters: domain_left_edge          = [0. 0. 0.]
yt : [INFO     ] 2022-05-27 11:41:21,889 Parameters: domain_right_edge         = [1. 1. 1.]
yt : [INFO     ] 2022-05-27 11:41:21,889 Parameters: cosmological_simulation   = 0
>>> p = yt.SlicePlot(ds, "z", ("gas", "density"))
yt : [INFO     ] 2022-05-27 11:41:35,183 xlim = 0.000000 1.000000
yt : [INFO     ] 2022-05-27 11:41:35,183 ylim = 0.000000 1.000000
yt : [INFO     ] 2022-05-27 11:41:35,183 xlim = 0.000000 1.000000
yt : [INFO     ] 2022-05-27 11:41:35,183 ylim = 0.000000 1.000000
yt : [INFO     ] 2022-05-27 11:41:35,184 Making a fixed resolution buffer of (('gas', 'density')) 800 by 800
>>> p.annotate_contour(("gas", "density"), invalid_kwarg=True)
<yt.visualization.plot_window.AxisAlignedSlicePlot object at 0x1276e7f70>
>>> p.save("/tmp/")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/_commons.py", line 146, in newfunc
    self._setup_plots()
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_window.py", line 1374, in _setup_plots
    self.run_callbacks()
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_window.py", line 1498, in run_callbacks
    callback = CallbackMaker(*args[1:], **kwargs)
TypeError: ContourCallback.__init__() got an unexpected keyword argument 'invalid_kwarg'

Expected outcome

The TypeError should show up as soon as we call the annotate_contour method.

This came up while I was trying to solve #3941: we have a bunch of deprecation warnings in callback methods, but they only pop at render time, so it's impossible to display the problematic line users actually typed in the warning itself.

I don't think this is a trivial fix because the current implementation for annotate_<x> methods is quite complicated and involves a lot of function wrapping. Any proposed solution will need to retain gains from #3310
Unless someone can find an elegant and compact solution soon, this shouldn't be required for yt 4.1

@neutrinoceros
Copy link
Member Author

update: turns out the same exact problem affects FIxedResolutionBuffer.apply_* methods, for similar reasons, so I'm fixing everything in one go in #3957

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better UX user-experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant