From f81459a30eb450813429a3c98de60d49a6e6f1b4 Mon Sep 17 00:00:00 2001
From: Kyle Conroy <kyleconroy@gmail.com>
Date: Fri, 22 Sep 2023 13:38:46 -0400
Subject: [PATCH] Binning: handle n_bins being empty or <= 0 (#51)

* avoid traceback in preview when n_bins is invalid form validation shows error message for widget already
* disable "bin" button for invalid inputs
* test coverage
---
 lcviz/plugins/binning/binning.py   | 16 +++++++++++++++-
 lcviz/plugins/binning/binning.vue  |  1 +
 lcviz/tests/test_plugin_binning.py | 18 ++++++++++++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/lcviz/plugins/binning/binning.py b/lcviz/plugins/binning/binning.py
index 5758d12b..30b9e15f 100644
--- a/lcviz/plugins/binning/binning.py
+++ b/lcviz/plugins/binning/binning.py
@@ -43,6 +43,7 @@ class Binning(PluginTemplateMixin, DatasetSelectMixin, EphemerisSelectMixin, Add
     show_live_preview = Bool(True).tag(sync=True)
 
     n_bins = IntHandleEmpty(100).tag(sync=True)
+    bin_enabled = Bool(True).tag(sync=True)
 
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
@@ -130,9 +131,18 @@ def viewer_filter(viewer):
     def _live_update(self, event={}):
         if not self.show_live_preview or not self.is_active:
             self._clear_marks()
+            self.bin_enabled = self.n_bins != '' and self.n_bins > 0
             return
 
-        lc = self.bin(add_data=False)
+        try:
+            lc = self.bin(add_data=False)
+        except Exception:
+            self._clear_marks()
+            self.bin_enabled = False
+            return
+        else:
+            self.bin_enabled = True
+
         # TODO: remove the need for this (inconsistent quantity vs value setting in lc object)
         lc_time = getattr(lc.time, 'value', lc.time)
 
@@ -173,6 +183,10 @@ def _on_ephemeris_update(self, msg):
         self._live_update()
 
     def bin(self, add_data=True):
+
+        if self.n_bins == '' or self.n_bins <= 0:
+            raise ValueError("n_bins must be a positive integer")
+
         input_lc = self.input_lc
 
         lc = input_lc.bin(time_bin_size=(input_lc.time[-1]-input_lc.time[0]).value/self.n_bins)
diff --git a/lcviz/plugins/binning/binning.vue b/lcviz/plugins/binning/binning.vue
index cf5c89b4..5dbac90b 100644
--- a/lcviz/plugins/binning/binning.vue
+++ b/lcviz/plugins/binning/binning.vue
@@ -68,6 +68,7 @@
       :add_to_viewer_selected.sync="add_to_viewer_selected"
       action_label="Bin"
       action_tooltip="Bin data"
+      :action_disabled="!bin_enabled"
       @click:action="apply"
     ></plugin-add-results>
 
diff --git a/lcviz/tests/test_plugin_binning.py b/lcviz/tests/test_plugin_binning.py
index 034d58d3..2c8cf8cd 100644
--- a/lcviz/tests/test_plugin_binning.py
+++ b/lcviz/tests/test_plugin_binning.py
@@ -1,3 +1,5 @@
+import pytest
+
 from lcviz.marks import LivePreviewBinning
 
 
@@ -47,3 +49,19 @@ def test_plugin_binning(helper, light_curve_like_kepler_quarter):
         ephem.period = 1.222
 
         b.bin(add_data=True)
+
+        # setting to invalid n_bins will raise error
+        b.n_bins = 0
+        assert b._obj.bin_enabled is False
+        with pytest.raises(ValueError):
+            b.bin(add_data=False)
+
+        # the enabled state of the button should work with or without live previews enabled
+        b.show_live_preview = False
+        assert len(_get_marks_from_viewer(tv)) == 0
+        assert len(_get_marks_from_viewer(pv)) == 0
+        assert b._obj.bin_enabled is False
+        b.n_bins = 1
+        assert b._obj.bin_enabled is True
+        b.n_bins = ''
+        assert b._obj.bin_enabled is False