[Feature] Wiring frb buffers with plot_window #1877

cphyc · 2018-07-03T21:59:54Z

PR Summary

This wires the FixedResolutionBuffer objects to their parent plot_window (if any) so that the addition of a filter results in the invalidation of the plot window.

The PR also modified the apply_gauss_beam filter to use https://docs.scipy.org/doc/scipy-0.16.1/reference/generated/scipy.ndimage.filters.gaussian_filter.html.

PR Checklist

Code passes flake8 checker
New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

Example script

import yt
from yt.testing import fake_amr_ds

ds = fake_amr_ds(fields=["density", "temperature"])

fields = ['density', 'temperature']
p = yt.SlicePlot(ds, 'x', fields)
p.save('/tmp/', suffix='unsmoothed')

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

p.frb.apply_white_noise()
p.save('/tmp/', suffix='smoothed+whitenoise')

cphyc · 2018-07-03T22:32:11Z

There is a lot of room for improvement in this PR and I don't really like the way it is wired now, because the frb should in principle be independent of the plot window.

One way to improve that would be to expose the frb-bound filters at the plot window level, and invalidate the data and call the subsequent frb filter from the plot window instead of relying on the filter to invalidate the data.

ngoldbaum · 2018-07-06T14:12:51Z

yt/visualization/fixed_resolution_filters.py

+        # We need to invalidate the data to force the plot window to
+        # take the filter into account.
+        if frb.plot_window:
+            frb.plot_window._data_valid = False


What if FixedResolutionBuffer instances had a _data_valid attribute. We would need to refactor PlotWindow to store that state on the frb attribute but I think it would be a straightforward refactor.

yt/visualization/fixed_resolution.py

ngoldbaum · 2018-07-06T14:13:40Z

yt/visualization/fixed_resolution.py

        self.data = {}
-        FixedResolutionBuffer.__init__(self, data_source, bounds, buff_size,
-                                       antialias, periodic)
+        super(ParticleImageBuffer, self).__init__(*args, **kwargs)


Can you explicitly pass the arguments?

ngoldbaum · 2018-07-06T14:13:49Z

yt/visualization/fixed_resolution.py

@@ -555,11 +554,9 @@ class ParticleImageBuffer(FixedResolutionBuffer):
    buffer.

    """
-    def __init__(self, data_source, bounds, buff_size, antialias=True,
-                 periodic=False):
+    def __init__(self, *args, **kwargs):


Can we explicitly list the arguments?

yt/visualization/fixed_resolution_filters.py

ngoldbaum · 2018-07-06T14:15:18Z

yt/visualization/plot_window.py

@@ -240,6 +240,7 @@ def fget(self):
        def fset(self, value):
            self._frb = value
            self._data_valid = True
+            self._plot_valid = False


I don't think this is used anywhere

It is at line 751 of the same file.

Ah I see, this is a good cleanup then. Sorry for not looking at the whole file, just the diff.

yt/visualization/plot_window.py

ngoldbaum · 2018-07-06T15:58:02Z

yt/visualization/plot_window.py

@@ -665,6 +667,9 @@ def __init__(self, *args, **kwargs):
        self._splat_color = kwargs.pop("splat_color", None)
        PlotWindow.__init__(self, *args, **kwargs)

+    def data_is_valid(self):
+        return self._data_valid and self._frb and self._frb._data_valid


Now that you've refactored everything into this function, do you think we need both _data_valid and frb._data_valid? Can't we just use frb._data_valid and get rid of the attribute on the plot itself?

I also like to make helpers like this properties so you don't need to append the () every time you use them.

ngoldbaum · 2018-07-06T15:59:16Z

Awesome, the latest version is much cleaner!

ngoldbaum · 2018-07-10T16:31:38Z

doc/source/visualizing/callbacks.rst

@@ -777,3 +777,21 @@ Overplot the Path of a Ray
   p.annotate_ray(oray)
   p.annotate_ray(ray)
   p.save()
+
+
+Operating on the final image


I would call this "applying filters"

yt/visualization/fixed_resolution_filters.py

ngoldbaum · 2018-07-10T16:34:26Z

yt/visualization/plot_window.py

@@ -659,6 +667,10 @@ def __init__(self, *args, **kwargs):
        self._splat_color = kwargs.pop("splat_color", None)
        PlotWindow.__init__(self, *args, **kwargs)

+    @property
+    def data_is_valid(self):
+        return self._data_valid and self._frb and self._frb._data_valid


I commented on this before and I don't think you responded - can you explain why we need both plot._data_valid and _frb._data_valid? I'd prefer to only store this state in one place if possible.

I've updated the code removing plot._data_valid, let's see if the test pass.

cphyc · 2018-07-10T17:37:27Z

The logic behind _data_valid is somewhat broken, essentially because it was centered on the plot even though it should be centered on the FixedResolutionBuffer. I think we can either leave it as is (so with a plot._data_valid and a plot._frb._data_valid attributes) and open an issue to clean all this or start a more massive refactoring that will put the data validation steps onto the frb.

ngoldbaum · 2018-07-10T18:26:25Z

OK, fair enough. I guess back out that last change? I will try to take a look at this this week sometime and see if there's a way to clean this up. Would you be ok with me pushing directly to this branch?

cphyc · 2018-07-10T21:14:08Z

@ngoldbaum it would indeed be enough to revert c3ef894 and 4cead9d and I'm fine with you pushing directly on the branch.

matthewturk · 2019-07-19T14:44:08Z

@cphyc Other than the merge conflict, is this ready to go? I realize it's been a long time.

matthewturk · 2020-03-27T19:50:41Z

@cphyc Hey, what do you think about this?

cphyc · 2020-03-30T08:41:26Z

I think this is ready to go. I remember having to think thoroughly to make sense of the logic behind the validation process (which would likely require need to be refactored). Let me trigger the tests to check that.

cphyc · 2020-03-30T08:41:34Z

@yt-fido test this please.

neutrinoceros

I left some minor comments, obviously this has already been refined a lot and overall it looks great, I would love if we could get this merged for the 4.0 release.
The only change I wish to request is about the gaussian filter refactor and deprecation.

yt/visualization/tests/test_filters.py

doc/source/visualizing/callbacks.rst

yt/visualization/fixed_resolution_filters.py

yt/visualization/plot_container.py

When adding a filter to a fixed resolution buffer, we want the plot_window to be invalidated so that the plot is redrawn using the filter.

…ects without a data_is_valid attribute

for more information, see https://pre-commit.ci

ENH: expose FRB filtering API (remake #1877)

cphyc requested a review from ngoldbaum July 3, 2018 22:02

ngoldbaum reviewed Jul 6, 2018

View reviewed changes

yt/visualization/fixed_resolution.py Outdated Show resolved Hide resolved

ngoldbaum reviewed Jul 6, 2018

View reviewed changes

yt/visualization/fixed_resolution_filters.py Outdated Show resolved Hide resolved

ngoldbaum reviewed Jul 6, 2018

View reviewed changes

yt/visualization/plot_window.py Outdated Show resolved Hide resolved

ngoldbaum reviewed Jul 6, 2018

View reviewed changes

cphyc changed the title ~~[WIP] [Feature] Wiring frb buffers with plot_window~~ [Feature] Wiring frb buffers with plot_window Jul 6, 2018

cphyc force-pushed the feature/smoothing branch from 8935c1b to 0e2e8fb Compare July 7, 2018 13:31

cphyc mentioned this pull request Jul 8, 2018

[Feature] Custom FRB filter #1882

Closed

3 tasks

ngoldbaum reviewed Jul 10, 2018

View reviewed changes

ngoldbaum mentioned this pull request Jul 10, 2018

Add smooth callback #1875

Closed

matthewturk added the triage Triage needed label Mar 6, 2019

neutrinoceros requested changes Jun 18, 2020

View reviewed changes

neutrinoceros added yt-4.0 feature targeted for the yt-4.0 release new feature Something fun and new! viz: 2D and removed triage Triage needed labels Jun 18, 2020

cphyc force-pushed the feature/smoothing branch from 6b5cb7f to 275ba01 Compare September 1, 2020 12:43

cphyc and others added 24 commits August 29, 2021 18:42

Wire the filters to the plot window functionality

6ec13e3

When adding a filter to a fixed resolution buffer, we want the plot_window to be invalidated so that the plot is redrawn using the filter.

Simplify 2g gaussian filter

fcb0eb9

Add test

5543e46

Pass plot_window to all FixedResolutionbuffer

1dbbee2

Check on plot window side

dba66a3

DRY: Wrap everything in a single function

5d5c59f

More thorough checks

0fe8f28

Updating doc

873020a

Remote WIP code

d7ddc7a

Make data_is_valid a property

64a593a

Fix signature of frb

296637d

Fix doc

0b4b88c

Update doc to include comments

71b246d

Issue warning when using the nbeam param

7f0663a

Remove _data_valid from plot_window

05dae59

Oops

0089d55

Flaking, etc.

adb84a6

More descriptive test

303dcdc

Take comments into account

dbc54b3

Update yt/visualization/fixed_resolution_filters.py

7be7bdb

bugfix: fix an error where validate_plot decorator would fail for obj…

f4ee761

…ects without a data_is_valid attribute

exp: deactivate frb data invalidation (should pass tests)

38d569f

[pre-commit.ci] auto fixes from pre-commit.com hooks

5227e6d

for more information, see https://pre-commit.ci

fix a broken import statement

166afce

neutrinoceros force-pushed the feature/smoothing branch from 15c0ff5 to 166afce Compare August 29, 2021 16:44

matthewturk dismissed neutrinoceros’s stale review via 166afce November 1, 2021 16:17

neutrinoceros mentioned this pull request Jan 21, 2022

ENH: expose FRB filtering API (remake #1877) #3756

Merged

3 tasks

cphyc closed this Feb 15, 2022

cphyc deleted the feature/smoothing branch February 15, 2022 13:39

chrishavlin added a commit that referenced this pull request May 2, 2022

Merge pull request #3756 from neutrinoceros/resurect_1877

c0241ea

ENH: expose FRB filtering API (remake #1877)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Wiring frb buffers with plot_window #1877

[Feature] Wiring frb buffers with plot_window #1877

cphyc commented Jul 3, 2018 •

edited by neutrinoceros

Loading

cphyc commented Jul 3, 2018

ngoldbaum Jul 6, 2018

ngoldbaum Jul 6, 2018

ngoldbaum Jul 6, 2018

ngoldbaum Jul 6, 2018

cphyc Jul 6, 2018

ngoldbaum Jul 6, 2018

ngoldbaum Jul 6, 2018

ngoldbaum commented Jul 6, 2018

ngoldbaum Jul 10, 2018

ngoldbaum Jul 10, 2018

cphyc Jul 10, 2018

cphyc commented Jul 10, 2018 •

edited

Loading

ngoldbaum commented Jul 10, 2018

cphyc commented Jul 10, 2018

matthewturk commented Jul 19, 2019

matthewturk commented Mar 27, 2020

cphyc commented Mar 30, 2020

cphyc commented Mar 30, 2020

neutrinoceros left a comment •

edited

Loading

[Feature] Wiring frb buffers with plot_window #1877

[Feature] Wiring frb buffers with plot_window #1877

Conversation

cphyc commented Jul 3, 2018 • edited by neutrinoceros Loading

PR Summary

PR Checklist

Example script

cphyc commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngoldbaum commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cphyc commented Jul 10, 2018 • edited Loading

ngoldbaum commented Jul 10, 2018

cphyc commented Jul 10, 2018

matthewturk commented Jul 19, 2019

matthewturk commented Mar 27, 2020

cphyc commented Mar 30, 2020

cphyc commented Mar 30, 2020

neutrinoceros left a comment • edited Loading

Choose a reason for hiding this comment

cphyc commented Jul 3, 2018 •

edited by neutrinoceros

Loading

cphyc commented Jul 10, 2018 •

edited

Loading

neutrinoceros left a comment •

edited

Loading