ENH: expose FRB filtering API (remake #1877)

[neutrinoceros]

PR Summary

This is an attempt at salvaging @cphyc's work in #1877. Last time, I recklessly rebased the original branch and created a mess.
This time, I copied the branch's history and rebased it on the current tip of the main branch.
Now there's ~0% chance this is going to pass CI on first try, but at least I can experiment on solutions without butchering the original code.

Here's an updated and simplified version of the demo script

import yt
from yt.testing import fake_amr_ds

field = ('gas', 'velocity')
ds = fake_amr_ds(fields=[field], units=["km/s"])

p = yt.SlicePlot(ds, 'x', field)
p.save('/tmp/unsmoothed.png')

p.frb.apply_gauss_beam(sigma=10) # pixels
p.refresh()
p.save('/tmp/smoothed.png')

p.frb.apply_white_noise()
p.refresh()
p.save('/tmp/smoothed+whitenoise.png')

Extra note: I'm deliberately trying to prune accessory ideas that are not essential to the feature (like revising the gaussian beam API). I do believe these ideas are good, but I'm just trying to make the core feature work again for now.

TODO:

• pass CI
• fix the demo script (right now the transformations are not applied)
• cleanup

[neutrinoceros]

right now I'm stuck on yt/visualization/tests/test_plotwindow.py::test_set_unit, where we see that custom units are not retained after a second data-invalidating operation is performed.

I've been attempting to solve this via refactors and I think that what I'm currently missing is a clear understanding of how the @invalidate_data/invalidate_plot decorators are supposed to work on PWViewerPlot.set_unit. I'll keep digging

[neutrinoceros]

I think I've reached a stage where the one failure is the new test, so I'm making progress.

[neutrinoceros]

I think this is almost ready now (in the sense that I now expect CI to go green) though I still need to clean up the history and probably add some more documentation.
In the current state there are 2 keys differences with respect to the original PR:

• I'm not introducing breaking changes to the gaussian beam filter
• users are required to call p.refresh() to actualise the plot itself after applying filters. This may be considered somewhat inelegant and possibly impractical though it seems like a much more maintainable solution than the alternative (doing it automatically). Given the original PR has now been stuck for 4 years, I'm hoping that this compromise can be considered acceptable.

[cphyc]

Should this go to the house of commons?

[neutrinoceros]

It's in line with the rest of the decorators I've moved there, but my original motivation was to make them easily importable from other modules, which I ended up not doing, so it would feel equally right to revert that code migration. Whatever you or other reviewers feel is preferable suits me :)

[neutrinoceros]

@cphyc would you agree to having this PR merged instead of the original one or should I close this and replay my changes there ? I would also love having your opinion on the proposed API changes (less practical from the user side, favoring maintability for now).

[cphyc]

Appart from documenting where manual refreshing needs to be used, this looks good to me.

[cphyc]

Here would be a good place to mention that the user may need to call p.refresh() for the plot to be updated.

[neutrinoceros]

Just added the missing bit of documentation.
I also ran the example for good measured and it seems that I accidentally changed the results from gaussian filtering, so I'll revert the existing implementation, which gives:

I am still slightly surprised by the result but fixing that is beyond the scope of the PR.

[chrishavlin]

This is great! Found one issue that I'm not sure is related directly to this PR but I think it should be addressed here. I'm also wondering if it's worth adding info in the docs on how to add filters dynamically...

the issue (arg!):

I wanted to understand how one would add a new frb filter, so I tried adding one but it looks like the filter api does not work with frb filters that accept args. I added the following:

class FixedResolutionBufferTrivialFilter(FixedResolutionBufferFilter):

    _filter_name = "trivial_filter"

    def __init__(self, cutoff):
        self.cutoff = cutoff

    def apply(self, buff):
        buff[buff > self.cutoff] = self.cutoff
        return buff

To yt.visualization.fixed_resolution_filters and then

import yt
from yt.testing import fake_amr_ds
import numpy as np

field = ('gas', 'velocity')
ds = fake_amr_ds(fields=[field], units=["km/s"])

p = yt.SlicePlot(ds, 'x', field)
p.frb.apply_trivial_filter(1e-2)
p.refresh()
p.save("test_trivial.png")

fails with:

< --- trimmed --- > 
~/src/yt_general/yt/yt/visualization/fixed_resolution.py in __getitem__(self, item)
    168 
    169         for name, (args, kwargs) in self._filters:
--> 170             buff = filter_registry[name](*args[1:], **kwargs).apply(buff)
    171 

TypeError: __init__() missing 1 required positional argument: 'cutoff'

I know this PR didn't touch that line 170, but why is it indexing args[1:] there? I changed that line to:

buff = filter_registry[name](*args, **kwargs).apply(buff)

And my trivial filter worked as expected.

extending filter behavior

Has there been any thought of creating more of a dynamic hook for frb filters? I'm not sure it's an intended consequence of this PR, but it's now super easy to add new filters without modifying source code. For example, the following totally works in a notebook (with the above fix for that line 170 args issue):

import yt
from yt.testing import fake_amr_ds
import numpy as np

field = ('gas', 'velocity')
ds = fake_amr_ds(fields=[field], units=["km/s"])

from scipy.ndimage import gaussian_filter
from yt.visualization.fixed_resolution_filters import FixedResolutionBufferFilter

class MyCustomFilter(FixedResolutionBufferFilter):

    _filter_name = "my_custom_filter"

    def __init__(self, *args, **kwargs):
        self.f_args = args
        self.f_kwargs = kwargs

    def apply(self, buff):
        return gaussian_filter(buff, *self.f_args, **self.f_kwargs)
    
p = yt.SlicePlot(ds, 'x', field)
p.frb.apply_my_custom_filter(5)
p.show()

results in:

Is it worth pointing this out in the docs? Or maybe a followup PR that would streamline and constrain this more intentionally?

[neutrinoceros]

Thanks for reviewing ! I didn't know about the issue you're pointing to, but I'll get back to you (unless @cphyc wants to answer first).

I can however clarify that the fact that you can extend the filter api dynamically from the user side is not a consequence of this PR, and it's true even on the main branch. I don't know wether it was intended, but I agree that it's a super cool fact about our API and it's definitely worth mentioning in the docs. I'll go as far as say that I don't think we need any modifications to the code base to advertise this extensibility. IMO it can be done now in a separate PR, and it doesn't need to be a follow up to this one.

[neutrinoceros]

but why is it indexing args[1:] there?

@chrishavlin so I think I figured it out; My hypothesis is that this was done to filter out the "self" argument, but this has become unnecessary (or maybe it always was) with how the _filters registry is now implemented. I think the reason why we never found this bug up to now is that the we've only advertised calling filters with keyword syntax (which seems somewhat more natural). I believe your patch isn't a hack but a bugfix. I'll include it in the present PR and you can merge then if you wish. If for any reason you prefer not to, we can also fast track the one liner bug fix as a separate PR.

[chrishavlin]

Great! Ya, I agree that using keywords is preferable, but having functional args is nice for wrapping functions. In any case, I don't see a need for another PR, fine to include the fix here IMO. There was an approval from @cphyc , should I wait for a re-review or go ahead and merge?

[neutrinoceros]

Corentin is the real author of the PR and my contribution was just a more involved approval. I think you can merge :)