From 90e3c80f81d223fad59441d0d981c2a9a205af2c Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 30 Apr 2025 16:54:17 +0100 Subject: [PATCH 1/9] make check_training_data_exists more strict, and test it --- cellfinder/napari/curation.py | 9 +++++---- tests/napari/test_curation.py | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index e480c412..38b14eee 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -496,23 +496,24 @@ def check_image_data_for_extraction(self) -> bool: def check_training_data_exists(self) -> bool: if not ( - self.training_data_cell_layer or self.training_data_non_cell_layer + self.training_data_cell_layer and self.training_data_non_cell_layer ): show_info( "No training data layers have been added. " - "Please add a layer and annotate some points.", + "Please add layers for both cells and non-cells," + "and annotate some points in both.", ) return False else: if ( len(self.training_data_cell_layer.data) > 0 - or len(self.training_data_non_cell_layer.data) > 0 + and len(self.training_data_non_cell_layer.data) > 0 ): return True else: show_info( "No training data points have been added. " - "Please annotate some points.", + "Please annotate points in both training data layers.", ) return False diff --git a/tests/napari/test_curation.py b/tests/napari/test_curation.py index ad760792..4975259c 100644 --- a/tests/napari/test_curation.py +++ b/tests/napari/test_curation.py @@ -202,3 +202,26 @@ def test_check_layer_removal_sync(valid_curation_widget): assert valid_curation_widget.background_layer is None assert valid_curation_widget.training_data_cell_layer is None assert valid_curation_widget.training_data_non_cell_layer is None + + +# @pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) +@pytest.mark.parametrize( + "layer_indices_to_remove", + [[-2], [-3], [-2, -3]], + ids=( + "users deletes non-cell training data layer", + "user deletes cell training data layer", + "user deletes both", + ), +) +def test_training_data_does_not_exist_when_user_removes_layers( + valid_curation_widget, layer_indices_to_remove +): + for layer in layer_indices_to_remove: + valid_curation_widget.viewer.layers.pop(layer) + assert not valid_curation_widget.check_training_data_exists() + + +# @pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) +def test_valid_widget_has_valid_training_data(valid_curation_widget): + assert valid_curation_widget.check_training_data_exists() From 86c447748d76854c4eb632850bec8f01881e6b03 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 30 Apr 2025 17:01:21 +0100 Subject: [PATCH 2/9] docstring for check_training_data_exists --- cellfinder/napari/curation.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index 38b14eee..d17b8c39 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -495,6 +495,10 @@ def check_image_data_for_extraction(self) -> bool: return False def check_training_data_exists(self) -> bool: + """ + Returns true both training layers exist, and have len > 0. + Otherwise shows returns False. + """ if not ( self.training_data_cell_layer and self.training_data_non_cell_layer ): From e732694b275c2876c2f37a0c93d6912316b04ee4 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 30 Apr 2025 17:09:35 +0100 Subject: [PATCH 3/9] improve docstring --- cellfinder/napari/curation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index d17b8c39..9e6f76c7 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -496,8 +496,8 @@ def check_image_data_for_extraction(self) -> bool: def check_training_data_exists(self) -> bool: """ - Returns true both training layers exist, and have len > 0. - Otherwise shows returns False. + Returns true if both training layers exist, and have len > 0. + Otherwise displays useful explanatory info and returns False. """ if not ( self.training_data_cell_layer and self.training_data_non_cell_layer From 5e4b1271723a099625068c8f8e8e764729e0a04a Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 30 Apr 2025 17:12:50 +0100 Subject: [PATCH 4/9] add xfail to new tests, due to #443 --- tests/napari/test_curation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/napari/test_curation.py b/tests/napari/test_curation.py index 4975259c..b2a30e75 100644 --- a/tests/napari/test_curation.py +++ b/tests/napari/test_curation.py @@ -204,7 +204,7 @@ def test_check_layer_removal_sync(valid_curation_widget): assert valid_curation_widget.training_data_non_cell_layer is None -# @pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) +@pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) @pytest.mark.parametrize( "layer_indices_to_remove", [[-2], [-3], [-2, -3]], @@ -222,6 +222,6 @@ def test_training_data_does_not_exist_when_user_removes_layers( assert not valid_curation_widget.check_training_data_exists() -# @pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) +@pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) def test_valid_widget_has_valid_training_data(valid_curation_widget): assert valid_curation_widget.check_training_data_exists() From 84b7473649fe00a56834bd620523b1357bfb8734 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 21 Jan 2026 13:22:19 +0000 Subject: [PATCH 5/9] loosen strictness of training data check --- cellfinder/napari/curation.py | 57 ++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index 9e6f76c7..aecaf53e 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -496,30 +496,57 @@ def check_image_data_for_extraction(self) -> bool: def check_training_data_exists(self) -> bool: """ - Returns true if both training layers exist, and have len > 0. - Otherwise displays useful explanatory info and returns False. + Checks that + - at least one training data layers exists + - it contains annotated points. + + If only one training layer exists, an info message is shown, + but this is considered valid. If no training layers exist or + all training layers are empty, a warning is displayed. + + Returns + ------- + bool + True if at least one training data layer containing points exists, + False otherwise. """ - if not ( + both_training_layers_exist = ( self.training_data_cell_layer and self.training_data_non_cell_layer - ): + ) + at_least_one_training_layer_exists = ( + self.training_data_cell_layer or self.training_data_non_cell_layer + ) + at_least_one_training_layer_contains_data = ( + len(self.training_data_cell_layer.data) > 0 + or len(self.training_data_non_cell_layer.data) > 0 + ) + + if not both_training_layers_exist: + # we don't want to fully prohibit this situation + # to allow users to start with just one layer + # but at the same time nudge users towards having + # both layers when doing "real" retraining show_info( + "Ensure you have both a cell and a non-cell layer with " + "roughly equal number of points for a balanced (re-)training" + ) + + if not at_least_one_training_layer_exists: + display_warning( "No training data layers have been added. " "Please add layers for both cells and non-cells," "and annotate some points in both.", ) return False + + if at_least_one_training_layer_contains_data: + return True else: - if ( - len(self.training_data_cell_layer.data) > 0 - and len(self.training_data_non_cell_layer.data) > 0 - ): - return True - else: - show_info( - "No training data points have been added. " - "Please annotate points in both training data layers.", - ) - return False + display_warning( + "No training data points have been added. " + "Please annotate points in both training data layers.", + ) + return False def get_output_directory(self): """ From 72b9f8fc292161d5fb2e87b28785b94ea867c98c Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 21 Jan 2026 17:12:30 +0000 Subject: [PATCH 6/9] make training data check logic very explicit --- cellfinder/napari/curation.py | 45 +++++++++++++++++---------------- tests/napari/test_curation.py | 47 +++++++++++++++++++++++------------ 2 files changed, 54 insertions(+), 38 deletions(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index aecaf53e..efbc91f2 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -11,7 +11,7 @@ from magicgui.widgets import ProgressBar from napari.qt.threading import thread_worker from napari.utils.notifications import show_info -from qt_niu.dialog import display_warning +from qt_niu.dialog import display_info, display_warning from qt_niu.interaction import add_button, add_combobox from qtpy import QtCore from qtpy.QtWidgets import ( @@ -513,37 +513,38 @@ def check_training_data_exists(self) -> bool: both_training_layers_exist = ( self.training_data_cell_layer and self.training_data_non_cell_layer ) - at_least_one_training_layer_exists = ( - self.training_data_cell_layer or self.training_data_non_cell_layer - ) - at_least_one_training_layer_contains_data = ( - len(self.training_data_cell_layer.data) > 0 - or len(self.training_data_non_cell_layer.data) > 0 - ) if not both_training_layers_exist: - # we don't want to fully prohibit this situation - # to allow users to start with just one layer - # but at the same time nudge users towards having - # both layers when doing "real" retraining - show_info( - "Ensure you have both a cell and a non-cell layer with " - "roughly equal number of points for a balanced (re-)training" - ) - - if not at_least_one_training_layer_exists: - display_warning( - "No training data layers have been added. " + display_info( + self, + "No training data layers have been added.", "Please add layers for both cells and non-cells," "and annotate some points in both.", ) return False + at_least_one_training_layer_contains_data = ( + len(self.training_data_cell_layer.data) > 0 + or len(self.training_data_non_cell_layer.data) > 0 + ) + + both_training_layers_contain_data = ( + len(self.training_data_cell_layer.data) > 0 + and len(self.training_data_non_cell_layer.data) > 0 + ) + if at_least_one_training_layer_contains_data: + if not both_training_layers_contain_data: + show_info( + "One of the training layers is empty. For optimal " + "(re-)training ensure you have roughly equal number " + "of points in each of your training points layers." + ) return True else: - display_warning( - "No training data points have been added. " + display_info( + self, + "No training data points have been added.", "Please annotate points in both training data layers.", ) return False diff --git a/tests/napari/test_curation.py b/tests/napari/test_curation.py index b2a30e75..db7a5f9f 100644 --- a/tests/napari/test_curation.py +++ b/tests/napari/test_curation.py @@ -170,12 +170,22 @@ def test_check_image_data_missing_signal(valid_curation_widget): ) -def test_is_data_extractable(curation_widget, valid_curation_widget): - """Check is_data_extractable works as expected.""" - assert not curation_widget.is_data_extractable() +def test_valid_widget_has_extractable_data(valid_curation_widget): + """Check is_data_extractable works as expected + when the widget has data. + """ assert valid_curation_widget.is_data_extractable() +def test_widget_without_data_is_not_extractable(curation_widget, mocker): + """Check is_data_extractable is False and shows info when the widget + does not have data set up. + """ + mock_info_popup = mocker.patch("cellfinder.napari.curation.display_info") + assert not curation_widget.is_data_extractable() + mock_info_popup.assert_called_once() + + def test_get_output_directory(valid_curation_widget): """Check get_output_directory returns expected value.""" with patch( @@ -204,24 +214,29 @@ def test_check_layer_removal_sync(valid_curation_widget): assert valid_curation_widget.training_data_non_cell_layer is None -@pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) -@pytest.mark.parametrize( - "layer_indices_to_remove", - [[-2], [-3], [-2, -3]], - ids=( - "users deletes non-cell training data layer", - "user deletes cell training data layer", - "user deletes both", - ), -) def test_training_data_does_not_exist_when_user_removes_layers( - valid_curation_widget, layer_indices_to_remove + valid_curation_widget, mocker ): - for layer in layer_indices_to_remove: + for layer in ("Training data (cells)", "Training data (non cells)"): valid_curation_widget.viewer.layers.pop(layer) + + mock_info_popup = mocker.patch("cellfinder.napari.curation.display_info") assert not valid_curation_widget.check_training_data_exists() + mock_info_popup.assert_called_once() -@pytest.mark.xfail(reason="See discussion in #443", raises=AssertionError) def test_valid_widget_has_valid_training_data(valid_curation_widget): assert valid_curation_widget.check_training_data_exists() + + +def test_show_info_called_on_empty_training_layer( + valid_curation_widget, mocker +): + mock_info_notification = mocker.patch( + "cellfinder.napari.curation.show_info" + ) + valid_curation_widget.viewer.layers["Training data (non cells)"].data = ( + None + ) + valid_curation_widget.check_training_data_exists() + mock_info_notification.assert_called_once() From e697eeb7b4516572970d8f1691b92c2ca590f8f4 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 21 Jan 2026 17:19:41 +0000 Subject: [PATCH 7/9] tweak docsting and user messages --- cellfinder/napari/curation.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index efbc91f2..57487462 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -497,18 +497,18 @@ def check_image_data_for_extraction(self) -> bool: def check_training_data_exists(self) -> bool: """ Checks that - - at least one training data layers exists - - it contains annotated points. + - both training data layers exists + - at least one of them is not empty. - If only one training layer exists, an info message is shown, - but this is considered valid. If no training layers exist or - all training layers are empty, a warning is displayed. + Will display a popup dialog if these conditions are not fulfilled. + Will show a notification if only one layer is non-empty, but this + is considered valid. Returns ------- bool - True if at least one training data layer containing points exists, - False otherwise. + True if both training layers exists and at least one + of them contains some data. False otherwise. """ both_training_layers_exist = ( self.training_data_cell_layer and self.training_data_non_cell_layer @@ -519,7 +519,7 @@ def check_training_data_exists(self) -> bool: self, "No training data layers have been added.", "Please add layers for both cells and non-cells," - "and annotate some points in both.", + "and annotate some points.", ) return False @@ -536,16 +536,16 @@ def check_training_data_exists(self) -> bool: if at_least_one_training_layer_contains_data: if not both_training_layers_contain_data: show_info( - "One of the training layers is empty. For optimal " - "(re-)training ensure you have roughly equal number " - "of points in each of your training points layers." + "One of the training layers is empty. This is OK, but" + "For optimal (re-)training ensure you have roughly equal " + "number of points in each of your training points layers." ) return True else: display_info( self, "No training data points have been added.", - "Please annotate points in both training data layers.", + "Please annotate points in the training data layers.", ) return False From 8a4a1a81ac972a6034b76ae5e5692ac75f28d2a0 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Wed, 21 Jan 2026 17:21:11 +0000 Subject: [PATCH 8/9] tweak docstring further --- cellfinder/napari/curation.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cellfinder/napari/curation.py b/cellfinder/napari/curation.py index 57487462..d46d7e4f 100644 --- a/cellfinder/napari/curation.py +++ b/cellfinder/napari/curation.py @@ -500,7 +500,9 @@ def check_training_data_exists(self) -> bool: - both training data layers exists - at least one of them is not empty. - Will display a popup dialog if these conditions are not fulfilled. + Will display a popup dialog and return False if these conditions + are not both fulfilled. + Will show a notification if only one layer is non-empty, but this is considered valid. From 0c1a8fb524e98284cc1d84b0fdff7269d3c73418 Mon Sep 17 00:00:00 2001 From: alessandrofelder Date: Thu, 22 Jan 2026 18:06:41 +0000 Subject: [PATCH 9/9] add additional assert to test --- tests/napari/test_curation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/napari/test_curation.py b/tests/napari/test_curation.py index db7a5f9f..7c5a4779 100644 --- a/tests/napari/test_curation.py +++ b/tests/napari/test_curation.py @@ -238,5 +238,5 @@ def test_show_info_called_on_empty_training_layer( valid_curation_widget.viewer.layers["Training data (non cells)"].data = ( None ) - valid_curation_widget.check_training_data_exists() + assert valid_curation_widget.check_training_data_exists() mock_info_notification.assert_called_once()