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

RFC: refactor plot callbacks registration #3957

Merged
merged 8 commits into from
Jun 14, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jun 4, 2022

PR Summary

Fix #3945
Right now this is a proof of concept, there's still work to do, but I think it's promising so I'm opening to see how many tests are currently broken.
It should backwards compatible, and retain compatibility with externally defined plot callbacks (such as HaloCatalogCallback from yt_astro_analysis).

If I can manage to get it 100% complete while keeping a reasonable diff size, I'll propose this for yt 4.1

Note: I ended up cleaning up a lot of signatures in PlotCallback subclasses __init__ methods (making most arguments keyword only). This change is technically not backward compatible. It is motivated by future compatibility concerns: some arguments are already deprecated, but sometimes occupy early positions, so there is no way to actually remove them in the future without breaking backward compatibility. Making them keyword only now is the least painful route I can see.
This wasn't the original goal of this PR, however it turned out that to be a requirement to keep this as tidy as possible, I'm happy to discuss it in more detail upon request.

In short, here's what works on the main branch but not here:

import yt

ds = yt.load_sample("IsolatedGalaxy")
p = yt.SlicePlot(ds, "z", ("gas", "density"))
p.annotate_contour(("gas", "density"), 5, 4, (1e-28, 1e-25))
p.save("/tmp/test_contour.png")

on this branch it will raise a TypeError (this is 100% by design)

Traceback (most recent call last):
  File "/Users/robcleme/dev/yt-project/yt/t_callback_validation.py", line 5, in <module>
    p.annotate_contour(("gas", "density"),5,4, (1e-28, 1e-25))
  File "/Users/robcleme/dev/yt-project/yt/yt/visualization/plot_modifications.py", line 103, in closure
    self._callbacks.append(cls(*args, **kwargs))
TypeError: ContourCallback.__init__() takes from 2 to 3 positional arguments but 5 were given

The fix is simply to use keyword arguments instead of positional:

p.annotate_contour(("gas", "density"), levels=5, factor=4, clim=(1e-28, 1e-25))

(in this particular example, the second argument, levels, main still be passed positionally)
Hopefully this is not too concerning of a change, since most arguments were desinged to be used as keyword rather than positional, and that's how they've always been demonstrated in docs. The main reason why they were not already implemented as keyword only is that this wasn't a thing in Python 2, but it's been around since Python 3.0

TODO:

PR Checklist

  • [N/A] New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@neutrinoceros neutrinoceros added refactor improve readability, maintainability, modularity UX user-experience labels Jun 4, 2022

callback_registry = {}
callback_registry: Dict[str, Type["PlotCallback"]] = {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this registry becomes less relevant with this patch, since we don't rely on it to setup annotation methods.
However it's still used in some places (most notably in a docs helper script), so I'm keeping it.

…ion methods now fail immediately and don't block rendering
@neutrinoceros neutrinoceros force-pushed the rfc_callback_registration branch 4 times, most recently from 4d662e2 to 70b6a67 Compare June 4, 2022 19:12
@neutrinoceros neutrinoceros added the enhancement Making something better label Jun 4, 2022
@neutrinoceros neutrinoceros added the deprecation deprecate features or remove deprecated ones label Jun 5, 2022
@neutrinoceros neutrinoceros added this to the 4.1.0 milestone Jun 5, 2022
… is sufficient, make most callbacks init arguments keyword-only to maximize future compatibility (enable existing deprecations to be concluded)
…d callbacks. This time put the declarations in callback classes directly, so it's more extensible and maintainable
@neutrinoceros neutrinoceros force-pushed the rfc_callback_registration branch 2 times, most recently from c0db82e to 41e6aa3 Compare June 6, 2022 11:19
@neutrinoceros neutrinoceros marked this pull request as ready for review June 6, 2022 12:10
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems OK, but also lots more typing fixes than I'd have expected!

yt/visualization/fixed_resolution.py Show resolved Hide resolved
@neutrinoceros
Copy link
Member Author

Not really fixes actually, mypy wasn't complaining about their absence. I just figured I might as well encode what I learned while I was at it :)

Copy link
Member

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this as is. Very nice refactor!

@neutrinoceros
Copy link
Member Author

Thanks ! anyone wants to merge ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation deprecate features or remove deprecated ones enhancement Making something better refactor improve readability, maintainability, modularity UX user-experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot callbacks do not validate input parameters on call
3 participants