-
Notifications
You must be signed in to change notification settings - Fork 279
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
RFC: refactor plot callbacks registration #3957
Conversation
1926744
to
2165f4c
Compare
|
||
callback_registry = {} | ||
callback_registry: Dict[str, Type["PlotCallback"]] = {} |
There was a problem hiding this comment.
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.
30ee415
to
12dea67
Compare
…ion methods now fail immediately and don't block rendering
4d662e2
to
70b6a67
Compare
bebd6f0
to
d327328
Compare
…rather than __call__
… 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
c0db82e
to
41e6aa3
Compare
41e6aa3
to
b45be4e
Compare
There was a problem hiding this 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!
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 :) |
There was a problem hiding this 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!
Thanks ! anyone wants to merge ? |
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
fromyt_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:
on this branch it will raise a TypeError (this is 100% by design)
The fix is simply to use keyword arguments instead of positional:
(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:
PlotCallback
subclasses__call__
to__init__
annotate_halos
from yt_astro_analysis still works as expected (checked with https://yt-astro-analysis.readthedocs.io/en/latest/halo_plot_callbacks.html?highlight=annotate_halo#annotate_halos)PR Checklist