fix: removing a box filter did not clear the filter in some cases

paulmueller · paulmueller · commit 0f5b35f929e9 · 2026-02-18T20:15:05.000+01:00
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,3 +1,11 @@
+2.25.1
+ - fix: removing a box filter did not clear the filter in some cases
+ - ref: do not allow specifying filters in `FilterRay.get_dataset()`
+ - ref: unify hashing caching of datasets in `FilterRay.segments`
+ - ref: remove ambiguous `FilterRay.root_child` workaround
+ - ref: simplify `FilterRay.get_final_child`
+ - tests: improve `FilterRay` test coverage
+ - setup: bump dclab to 0.67.4 (correct `reset_filter` method)
 2.25.0
  - feat: increase width of dataset tile in block matrix via settings (#225)
  - fix: IndexError when trying to view the logs of a log-less dataset
diff --git a/dcscope/pipeline/core.py b/dcscope/pipeline/core.py
@@ -464,8 +464,9 @@ def get_dataset(self, slot_index, filt_index=-1, apply_filter=True):
             filters = self.get_filters_for_slot(slot_id=slot_id,
                                                 max_filter_index=filt_index)
             # filter ray magic
-            ray = self.get_ray(slot.identifier)
-            ds = ray.get_dataset(filters, apply_filter=apply_filter)
+            ray = self.get_ray(slot_id)
+            ray.set_filters(filters)
+            ds = ray.get_dataset(apply_filter=apply_filter)
         return ds
 
     def get_datasets(self, filt_index=-1, apply_filter=True):
diff --git a/dcscope/pipeline/filter_ray.py b/dcscope/pipeline/filter_ray.py
@@ -12,29 +12,28 @@ def __init__(self, slot):
         self.identifier = slot.identifier
         #: slot defining the ray
         self.slot = slot
-        #: list of RTDCBase (hierarchy children)
-        self.steps = []
-        #: corresponds to hashes of the applied filters
-        self.step_hashes = []
+        #: segments of the filter ray, consisting of hash, previous, and
+        #: next dataset
+        self.segments = []
         # holds the filters (protected so that users use set_filters)
         self._filters = []
         # used for testing (incremented when the ray is cut)
         self._generation = 0
         # used for checking validity of the ray
         self._slot_hash = "unset"
-        self._root_child = None
 
     def __repr__(self):
         repre = "<Pipeline Filter Ray '{}' at {}>".format(self.identifier,
                                                           hex(id(self)))
         return repre
 
-    def _add_step(self, ds, filt):
-        """Add a filter step"""
-        self.step_hashes.append(filt.hash)
+    def _add_segment(self, ds, filt):
+        """Add a filter segment"""
+        ds.reset_filter()
         filt.update_dataset(ds)
-        self.steps.append(ds)
-        return self._new_child(ds, filt)
+        child = self._new_child(ds, filt)
+        self.segments.append([filt.hash, ds, child])
+        return child
 
     def _new_child(self, ds, filt=None, apply_filter=False):
         identifier = self.slot.identifier
@@ -58,21 +57,7 @@ def filters(self):
         """
         return self._filters
 
-    @property
-    def root_child(self):
-        """This is the first element in self.steps
-        (Will return a dataset even if self.steps is empty)
-        """
-        if self._slot_hash != self.slot.hash:
-            # reset everything (e.g. emodulus recipe might have changed)
-            self.steps = []
-            self.step_hashes = []
-            self._root_child = self._new_child(self.slot.get_dataset(),
-                                               apply_filter=True)
-            self._slot_hash = self.slot.hash
-        return self._root_child
-
-    def get_final_child(self, rtdc_ds=None, apply_filter=True):
+    def get_final_child(self, rtdc_ds=None, filters=None, apply_filter=True):
         """Return the final ray child of `rtdc_ds`
 
         If `rtdc_ds` is None, then the dataset of the current
@@ -86,19 +71,27 @@ def get_final_child(self, rtdc_ds=None, apply_filter=True):
         is applied to other data on disk e.g. when computing
         statistics. For regular use of the filter ray in a
         pipeline, use :func:`get_dataset`.
+
+        .. versionchanged:: 2.25.1
+          The dataset returned is a clean child dataset without any
+          filters defined.
+
         """
-        filters = self.filters
+        if filters is None:
+            filters = self.filters
+            external_filt = False
+        else:
+            external_filt = True
 
         if rtdc_ds is None:
             # normal case
-            external = False
-            rtdc_ds = self.slot.get_dataset()
-            ds = self.root_child
+            external_ds = False
+            ds = self.slot.get_dataset()
         else:
             # ray is applied to other data
-            external = True
-            # do not modify rtdc_ds (create a child to work with)
-            ds = self._new_child(rtdc_ds, apply_filter=True)
+            external_ds = True
+            # do not modify the original dataset (create a child to work with)
+            ds = self._new_child(rtdc_ds)
 
         # Dear future self,
         #
@@ -107,48 +100,46 @@ def get_final_child(self, rtdc_ds=None, apply_filter=True):
         # Sincerely,
         # past self
 
+        filters = [f for f in filters if f.filter_used]
+
         if filters:
             # apply all filters
             for ii, filt in enumerate(filters):
                 # remember the previous hierarchy parent
                 # (ds is always used for the next iteration)
-                prev_ds = ds
-                if external:
-                    # do not touch self.steps or self.step_hashes
+                if external_ds or external_filt:
+                    # do not touch self.segments
                     filt.update_dataset(ds)
                     ds = self._new_child(ds, filt)
-                elif len(self.steps) < ii+1:
-                    # just create a new step
-                    ds = self._add_step(ds, filt)
-                elif filt.hash != self.step_hashes[ii]:
+                elif len(self.segments) < ii + 1:
+                    # just create a new segment
+                    ds = self._add_segment(ds, filt)
+                elif filt.hash != self.segments[ii][0]:
                     # the filter ray is changing here;
-                    # cut it and add a new step
-                    self.steps = self.steps[:ii]
-                    self.step_hashes = self.step_hashes[:ii]
-                    ds = self._add_step(ds, filt)
+                    # trim it and add a new segment
+                    self.segments = self.segments[:ii]
+                    ds = self._add_segment(ds, filt)
                     self._generation += 1  # for testing
                 else:
-                    # the filters match so far
-                    if len(self.steps) > ii + 1:  # next child exists
-                        ds = self.steps[ii + 1]
-                    else:  # next child does not exist
-                        ds = self._new_child(ds, filt)
-            # we now have the entire filter pipeline in self.steps
-            final_ds = prev_ds
+                    # reuse previous segment
+                    ds = self.segments[ii][2]
+            final_ds = ds
         else:
-            final_ds = rtdc_ds
+            final_ds = ds
+
+        if not external_ds:
+            ds.reset_filter()
+
         if apply_filter:
             final_ds.apply_filter()
+
         return final_ds
 
-    def get_dataset(self, filters=None, apply_filter=True):
+    def get_dataset(self, apply_filter=True):
         """Return the dataset that corresponds to applying these filters
 
         Parameters
         ----------
-        filters: list of Filter or None
-            Filters used for computing the dataset hierarchy. If set
-            to None, the current filters in `self.filters` are used.
         apply_filter: bool
             Whether to apply all filters and update the metadata of
             the requested dataset. This should be True if you are
@@ -157,14 +148,11 @@ def get_dataset(self, filters=None, apply_filter=True):
             apply some more filters and then call `rejuvenate`
             yourself.
         """
-        if filters is not None:
-            # put the filters in place
-            self.set_filters(filters)
         # compute the final hierarchy child
         ds = self.get_final_child(apply_filter=apply_filter)
         return ds
 
     def set_filters(self, filters):
         """Set the filters of the current ray"""
         # only take into account active filters
-        self._filters = [f for f in filters if f.filter_used]
+        self._filters = filters
diff --git a/pyproject.toml b/pyproject.toml
@@ -30,7 +30,7 @@ classifiers = [
 ]
 license = "GPL-3.0-or-later"
 dependencies = [
-    "dclab[dcor,export,http,s3]>=0.67.3",
+    "dclab[dcor,export,http,s3]>=0.67.4",
     "h5py>=2.8.0",
     "numpy>=1.21",  # CVE-2021-33430
     "pygments",
diff --git a/tests/test_gui_filter.py b/tests/test_gui_filter.py
@@ -101,6 +101,104 @@ def test_box_filter_selection_no_preselection_issue_67(qtbot):
     qtbot.mouseClick(wf.toolButton_moreless, QtCore.Qt.MouseButton.LeftButton)
 
 
+def test_filter_min_max_delete(qtbot):
+    """Create a box filter, apply it, and delete it again"""
+    path = make_fake_dataset()
+
+    mw = DCscope()
+    qtbot.addWidget(mw)
+
+    # add the file
+    mw.add_dataslot(paths=[path, path])
+
+    assert len(mw.pipeline.slot_ids) == 2, "we added that"
+    assert len(mw.pipeline.filter_ids) == 1, "automatically added"
+    # sanity check
+    ds10 = dclab.new_dataset(mw.pipeline.get_dataset(0))
+    ds20 = dclab.new_dataset(mw.pipeline.get_dataset(0))
+    assert np.max(ds10["area_um"]) > 20
+    assert np.max(ds20["area_um"]) > 20
+
+    # open the filter edit in the Analysis View
+    fe = mw.block_matrix.get_widget(filt_plot_id=mw.pipeline.filter_ids[0])
+    qtbot.mouseClick(fe.toolButton_modify, QtCore.Qt.MouseButton.LeftButton)
+
+    # box filtering
+    wf = mw.widget_ana_view.widget_filter
+    mw.widget_ana_view.tabWidget.setCurrentWidget(
+        mw.widget_ana_view.tab_filter)
+
+    # enable selection
+    qtbot.mouseClick(wf.toolButton_moreless, QtCore.Qt.MouseButton.LeftButton)
+    # find the porosity item and click the checkbox
+    rc = wf._box_range_controls["area_um"]
+    qtbot.mouseClick(rc.checkBox, QtCore.Qt.MouseButton.LeftButton)
+    # disable selection
+    qtbot.mouseClick(wf.toolButton_moreless, QtCore.Qt.MouseButton.LeftButton)
+
+    # set the range control for area from 20 to 30 µm
+    rc.doubleSpinBox_min.setValue(20)
+    rc.doubleSpinBox_max.setValue(30)
+
+    # click apply
+    qtbot.mouseClick(wf.pushButton_apply, QtCore.Qt.MouseButton.LeftButton)
+
+    # make sure this worked
+    assert rc.read_pipeline_state()["end"] == 30
+    box_filters = mw.pipeline.filters[0].__getstate__()["box filters"]
+    assert box_filters["area_um"]["end"] == 30
+    assert box_filters["area_um"]["active"] is True
+
+    # activate the filter for both datasets
+    me1 = mw.block_matrix.get_widget(filt_plot_id=mw.pipeline.filter_ids[0],
+                                     slot_id=mw.pipeline.slot_ids[0]
+                                     )
+    qtbot.mouseClick(me1, QtCore.Qt.MouseButton.LeftButton)
+    me2 = mw.block_matrix.get_widget(filt_plot_id=mw.pipeline.filter_ids[0],
+                                     slot_id=mw.pipeline.slot_ids[1]
+                                     )
+    qtbot.mouseClick(me2, QtCore.Qt.MouseButton.LeftButton)
+
+    QtWidgets.QApplication.processEvents(
+        QEventLoop.ProcessEventsFlag.AllEvents, 500)
+
+    assert len(mw.pipeline.get_filters_for_slot(mw.pipeline.slot_ids[0])) == 1
+    assert len(mw.pipeline.get_filters_for_slot(mw.pipeline.slot_ids[1])) == 1
+
+    # get the datasets from the pipeline and check that they are filtered
+    ds1a = dclab.new_dataset(mw.pipeline.get_dataset(0))
+    ds2a = dclab.new_dataset(mw.pipeline.get_dataset(1))
+
+    # This is expected
+    assert np.max(ds1a["area_um"]) < 30
+    assert np.max(ds2a["area_um"]) < 30
+
+    # Now, remove the box filter!
+    qtbot.mouseClick(wf.toolButton_moreless, QtCore.Qt.MouseButton.LeftButton)
+    # find the porosity item and click the checkbox
+    rc = wf._box_range_controls["area_um"]
+    qtbot.mouseClick(rc.checkBox, QtCore.Qt.MouseButton.LeftButton)
+    # disable selection
+    qtbot.mouseClick(wf.toolButton_moreless, QtCore.Qt.MouseButton.LeftButton)
+
+    # click apply
+    qtbot.mouseClick(wf.pushButton_apply, QtCore.Qt.MouseButton.LeftButton)
+    QtWidgets.QApplication.processEvents(
+        QEventLoop.ProcessEventsFlag.AllEvents, 500)
+
+    # make sure this worked
+    box_filters = mw.pipeline.filters[0].__getstate__()["box filters"]
+    assert "area_um" not in box_filters
+
+    # get the datasets from the pipeline and check that they are filtered
+    ds1b = dclab.new_dataset(mw.pipeline.get_dataset(0))
+    ds2b = dclab.new_dataset(mw.pipeline.get_dataset(1))
+
+    # This was the bug. The filters were not removed correctly.
+    assert np.max(ds1b["area_um"]) > 30
+    assert np.max(ds2b["area_um"]) > 30
+
+
 def test_filter_min_max_inf(qtbot):
     path = make_fake_dataset()
 
@@ -199,7 +297,7 @@ def test_polygon_filter_basic(qtbot):
     ds = mw.pipeline.get_dataset(0)
     assert ds_slot is not ds
     assert np.sum(ds.filter.all) == 5
-    assert len(ds) == 47
+    assert len(ds) == 5
 
 
 def test_polygon_filter_delete(qtbot):
diff --git a/tests/test_pipeline_filter_ray.py b/tests/test_pipeline_filter_ray.py

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ classifiers = [`
`30`	`30`	`]`
`31`	`31`	`license = "GPL-3.0-or-later"`
`32`	`32`	`dependencies = [`
`33`		`- "dclab[dcor,export,http,s3]>=0.67.3",`
	`33`	`+ "dclab[dcor,export,http,s3]>=0.67.4",`
`34`	`34`	`"h5py>=2.8.0",`
`35`	`35`	`"numpy>=1.21", # CVE-2021-33430`
`36`	`36`	`"pygments",`