Fix coupling between data manager and view

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 * "All" tab in data manager to allow searching through entire dataset.
+* Sample buttons for data manager rows that aren't already in sample.
 
 
 

icat/data.py

@@ -106,6 +106,7 @@ def __init__(
         self.table.on_select_point(self.fire_on_row_selected)
         self.table.on_select_point(self._handle_row_selected)
         self.table.on_add_example(self._handle_example_added)
+        self.table.on_add_to_sample(self._handle_sample_added)
 
         self.filtered_df = None
 
@@ -209,7 +210,6 @@ def _handle_ipv_label_all_u_btn_click(self, widget, event, data):
     # could have a "sample_changed" event that view listens to.
     def _handle_ipv_resample_btn_click(self, widget, event, data):
         self.set_random_sample()
-        self.model.view.refresh_data()
 
     def _handle_ipv_tab_changed(self, widget, event, data: int):
         """Event handler for the vuetify tabs change. This changes the current_data_tab
@@ -260,6 +260,10 @@ def _handle_example_added(self, point_id):
             # marginally cleaner and make model's interface nicer as well.
             self.model.anchor_list.add_anchor(new_anchor)
 
+    def _handle_sample_added(self, point_id):
+        """Event handler for when the 'sample' button is clicked."""
+        self.sample_indices = [*self.sample_indices, point_id]
+
     def _handle_label_changed(self, index: int | list[int], new_label: int | list[int]):
         if type(index) == list:
             for i in range(len(index)):
@@ -303,6 +307,7 @@ def on_row_selected(self, callback: Callable):
         """
         self._row_selected_callbacks.append(callback)
 
+    @param.depends("sample_indices", watch=True)
     def fire_on_sample_changed(self):
         for callback in self._sample_changed_callbacks:
             callback(self.sample_indices)
@@ -354,6 +359,10 @@ def _set_page_contents(self, options):
             # I think this function gets triggered on table widget init, which isn't
             # necessary, so just ignore
             return
+
+        if self.text_col is None:
+            return
+
         df = self.filtered_df
         page_num = 1
         if "page" in options:
@@ -410,7 +419,14 @@ def _set_page_contents(self, options):
                         labeled = "orange"
                     labeled = f"<span class='{labeled}--text darken-1'>Labeled</span>"
 
-            rows.append(dict(id=index, text=text, labeled=labeled))
+            rows.append(
+                dict(
+                    id=index,
+                    text=text,
+                    labeled=labeled,
+                    in_sample=(index in self.sample_indices),
+                )
+            )
         self.table.items = rows
 
     @staticmethod
@@ -524,4 +540,3 @@ def set_random_sample(self):
             self.sample_indices = list(self.active_data.sample(100).index)
         else:
             self.sample_indices = list(self.active_data.index)
-        self.fire_on_sample_changed()

icat/table.py

@@ -25,6 +25,7 @@ def __init__(self, *args, **kwargs):
         self._add_selected_text_callbacks: list[Callable] = []
         self._select_point_callbacks: list[Callable] = []
         self._apply_label_callbacks: list[Callable] = []
+        self._add_to_sample_callbacks: list[Callable] = []
         self._update_options_callbacks: list[Callable] = []
         self._hover_point_callbacks: list[Callable] = []
         self._add_example_callbacks: list[Callable] = []
@@ -42,6 +43,10 @@ def on_apply_label(self, callback: callable):
         """Expect a point id and a label value (0 or 1)"""
         self._apply_label_callbacks.append(callback)
 
+    def on_add_to_sample(self, callback: callable):
+        """Expect a point id"""
+        self._add_to_sample_callbacks.append(callback)
+
     def on_update_options(self, callback: callable):
         """Expect a dictionary with:
             * page: int
@@ -98,6 +103,10 @@ def vue_updateOptions(self, data):
         for callback in self._update_options_callbacks:
             callback(data)
 
+    def vue_addToSample(self, point_id):
+        for callback in self._add_to_sample_callbacks:
+            callback(point_id)
+
     # NOTE: I'm leaving this here for reference, I don't actually think there's any reason to need to go this direction
     # but this is a functioning way to do it (note that this will _retrigger_ the updateOptions handler)
     # def change_page(self, page_num):
@@ -133,6 +142,9 @@ def _template(self):
                             <v-btn x-small class="purple darken-1" @click.stop="addToExampleAnchor(item.id)">
                                 example
                             </v-btn>
+                            <v-btn x-small v-if="!item.in_sample" @click.stop="addToSample(item.id)">
+                                sample
+                            </v-btn>
                             <div v-html="item.labeled" />
                         </td>
                     </tr>

icat/view.py

@@ -19,6 +19,15 @@
 
 
 class InteractiveView(pn.viewable.Viewer):
+    """The GUI/widget dashboard for interacting with the model, data, and anchors.
+
+    This class glues all of the events together across the various components (anchorlist,
+    datamanager, ipyanchorviz etc.), and itself is the layout container for the full dashboard.
+
+    Args:
+        model (icat.Model): The parent model that this view is associated with.
+    """
+
     # TODO: coupling: technically coupling model and view, I think I care a lot less about it
     # here since view is already loosely orchestrating all of the gui stuff. If anything,
     # _more_ of the model event handlers should be handled in here instead of in the model?
@@ -58,11 +67,19 @@ def __init__(self, model, **params):
             self._trigger_selected_points_change, names="lassoedPointIDs"
         )
         self.model.data.table.on_point_hover(self._set_anchorviz_selected_point)
+        self.model.data.on_sample_changed(self._handle_data_sample_changed)
         self.histograms.on_range_changed(self._histograms_range_changed)
         super().__init__(**params)
         self.refresh_data()
 
+    def _handle_data_sample_changed(self, new_sample_indices: list[int]):
+        """When the model's data manager sample_indices changes, it fires the
+        on_sample_changed event."""
+        self.refresh_data()
+
     def _histograms_range_changed(self, range: list[int]):
+        """Limit the set of points displayed in anchorviz to only prediction
+        outputs within the specified range. [min, max]"""
         self.model.data.pred_min = range[0]
         self.model.data.pred_max = range[1]
         self.anchorviz.set_points(self._serialize_data_to_dicts())

tests/test_datamanager.py

@@ -4,6 +4,7 @@
 import pytest
 
 from icat import Model
+from icat.data import DataManager
 
 
 def test_manager_selected_indices_effects_selected_tab(dummy_data_manager):
@@ -143,3 +144,67 @@ def test_apply_multiple_labels_appends_model_training_data(fun_df):
 
     assert len(model.training_data) == 6
     assert model.training_data["_label"].values.tolist() == [1, 1, 0, 0, 0, 1]
+
+
+def test_datamanager_with_none_text_col_doesnot_error(fun_df):
+    """Technically it's allowed to have a data manager without a text col, it just
+    shouldn't show anything."""
+    DataManager(fun_df)
+
+
+def test_datamanager_with_none_data_doesnot_error():
+    """Technically it's allowed to have a data manager without any actual data, it just
+    shouldn't show anything."""
+    DataManager(None)
+
+
+def test_setting_sample_indices_fires_event(dummy_data_manager):
+    """Manually setting the sample of the data manager should fire the on_sample_changed event."""
+    returns = []
+
+    def catch_change(sample_indices):
+        nonlocal returns
+        returns.append(sample_indices)
+
+    dummy_data_manager.sample_indices = [1, 2, 3, 4, 5, 6]
+    dummy_data_manager.on_sample_changed(catch_change)
+    dummy_data_manager.sample_indices = [1, 2, 3]
+    assert len(returns) == 1
+    assert returns[0] == [1, 2, 3]
+
+
+@pytest.mark.integration
+def test_adding_single_point_to_sample_fires_event(dummy_data_manager):
+    """A sample added button click (from the data manager's event handler) should fire the
+    on_sample_changed event."""
+    returns = []
+
+    def catch_change(sample_indices):
+        nonlocal returns
+        returns.append(sample_indices)
+
+    dummy_data_manager.sample_indices = [1, 2, 3]
+    dummy_data_manager.on_sample_changed(catch_change)
+    dummy_data_manager.table.vue_addToSample(4)
+    assert len(returns) == 1
+    assert returns[0] == [1, 2, 3, 4]
+
+
+def test_resampling_fires_event(dummy_data_manager):
+    """Clicking the resample button should fire the on_sample_changed event."""
+    returns = []
+
+    def catch_change(sample_indices):
+        nonlocal returns
+        returns.append(sample_indices)
+
+    dummy_data_manager.sample_indices = [1, 2, 3]
+    dummy_data_manager.on_sample_changed(catch_change)
+    dummy_data_manager.set_random_sample()
+    assert len(returns) == 1
+    assert returns[0] == [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11]
+
+
+# @pytest.mark.integration
+# # def
+# """"""