From 5e3f64bf3d4156c3d2a9ac95afc21d4604b6491c Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 19 Jan 2022 12:48:55 -0500 Subject: [PATCH 1/4] handle empty poly order * traitlet now accepts the empty string, with form validation ensuring that it is never passed as such to python (can't click "add component" button while poly_order is empty) * if adopted, similar logic should be applied across all relevant plugin inputs --- docs/dev/ui_style_guide.rst | 36 +++++ .../plugins/model_fitting/model_fitting.py | 38 ++++-- .../plugins/model_fitting/model_fitting.vue | 123 ++++++++++-------- 3 files changed, 132 insertions(+), 65 deletions(-) diff --git a/docs/dev/ui_style_guide.rst b/docs/dev/ui_style_guide.rst index 3a584cdb03..43d9f0cbd6 100644 --- a/docs/dev/ui_style_guide.rst +++ b/docs/dev/ui_style_guide.rst @@ -25,6 +25,18 @@ try to adhere to the following principles: * Use ```` to align content to the right (such as action buttons). * Use new ``Header Text`` to separate content within a plugin (instead of nested cards, ``v-card-subtitle``, etc). +* Number entries should use a ```` component + *unless* requiring support for scientific notation (in which case + ```` can be used with stripping invalid characters and + type-casting in python). To handle emptying the input component without raising a traceback, + use an ``IntHandleEmpty`` traitlet instead, along with form-validation (see below) and/or + checks on the python-side to handle the case of an empty string. +* Use form validation wherever possible, and disable action buttons if the relevant validation + does not pass. This is preferred to raising errors through snackbars after pressing an action + button. To do this, wrap the relevant section in a ````, + create a ``form_valid_section_name = Bool(False).tag(sync=True)`` in the python class for the + plugin, add rules to any relevant inputs, and set ``:disabled="!form_valid_section_name"`` to any + action buttons. .. code:: @@ -37,5 +49,29 @@ try to adhere to the following principles: .... + + + + + + + + + Action Text + + + diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index a0bdca2aad..6547c87543 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -24,6 +24,23 @@ __all__ = ['ModelFitting'] +class IntHandleEmpty(Int): + def __init__(self, *args, **kwargs): + self._empty_to_default = kwargs.pop('replace_with_default', False) + super().__init__(*args, **kwargs) + + def validate(self, obj, value): + if isinstance(value, str) and not len(value): + if self._empty_to_default: + # if the field is emptied, it will override with the default value + return self.default_value + # the value will remain as the empty string, likely will need to either + # couple this with form validation or handle the case where the value + # is an empty string once retrieved. + return value + return super().validate(obj, value) + + class _EmptyParam: def __init__(self, value, unit=None): self.value = value @@ -37,7 +54,10 @@ class ModelFitting(TemplateMixin): dialog = Bool(False).tag(sync=True) template_file = __file__, "model_fitting.vue" dc_items = List([]).tag(sync=True) + form_valid_data_selection = Bool(False).tag(sync=True) + form_valid_model_component = Bool(False).tag(sync=True) + selected_data = Unicode("").tag(sync=True) spectral_min = Any().tag(sync=True) spectral_max = Any().tag(sync=True) spectral_unit = Unicode().tag(sync=True) @@ -52,7 +72,7 @@ class ModelFitting(TemplateMixin): eq_error = Bool(False).tag(sync=True) component_models = List([]).tag(sync=True) display_order = Bool(False).tag(sync=True) - poly_order = Int(0).tag(sync=True) + poly_order = IntHandleEmpty(0).tag(sync=True) # add/replace results for "fit" add_replace_results = Bool(True).tag(sync=True) @@ -79,7 +99,6 @@ def __init__(self, *args, **kwargs): self._initialized_models = {} self._display_order = False self.model_label = "Model" - self._selected_data_label = None self._spectral_subsets = {} self._window = None self._original_mask = None @@ -245,7 +264,8 @@ def _warn_if_no_equation(self): else: return False - def vue_data_selected(self, event): + @observe("selected_data") + def _selected_data_changed(self, event): """ Callback method for when the user has selected data from the drop down in the front-end. It is here that we actually parse and create a new @@ -260,7 +280,7 @@ def vue_data_selected(self, event): label of the data collection object selected by the user. """ selected_spec = self.app.get_data_from_viewer("spectrum-viewer", - data_label=event) + data_label=event['new']) # Replace NaNs from collapsed SpectralCube in Cubeviz # (won't affect calculations because these locations are masked) selected_spec.flux[np.isnan(selected_spec.flux)] = 0.0 @@ -268,8 +288,6 @@ def vue_data_selected(self, event): # Save original mask so we can reset after applying a subset mask self._original_mask = selected_spec.mask - self._selected_data_label = event - if self._units == {}: self._units["x"] = str( selected_spec.spectral_axis.unit) @@ -471,8 +489,8 @@ def vue_fit_model_to_cube(self, *args, **kwargs): if self._warn_if_no_equation(): return - if self._selected_data_label in self.app.data_collection.labels: - data = self.app.data_collection[self._selected_data_label] + if self.selected_data in self.app.data_collection.labels: + data = self.app.data_collection[self.selected_data] else: # User selected some subset from spectrum viewer, just use original cube data = self.app.data_collection[0] @@ -480,7 +498,7 @@ def vue_fit_model_to_cube(self, *args, **kwargs): # that the user has selected a pre-existing 1d data object. if data.ndim != 3: snackbar_message = SnackbarMessage( - f"Selected data {self._selected_data_label} is not cube-like", + f"Selected data {self.selected_data} is not cube-like", color='error', sender=self) self.hub.broadcast(snackbar_message) @@ -586,7 +604,7 @@ def vue_register_spectrum(self, event): self.app.add_data(spectrum, label) # Make sure we link the result spectrum to the data we're fitting - data_fitted = self.app.session.data_collection[self._selected_data_label] + data_fitted = self.app.session.data_collection[self.selected_data] data_id = data_fitted.world_component_ids[0] model_id = self.app.session.data_collection[label].world_component_ids[0] self.app.session.data_collection.add_link(LinkSame(data_id, model_id)) diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue index 4b54f3182d..a436c0eb35 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue @@ -4,67 +4,80 @@ Fit an analytic model to data or a subset. - - - + + + + - - - + + + + Model Components - - - + + + + - - - - + + + + - - - - + + + + - - - Add Component - - + + + Add Component + + + +
Model Parameters From 98b0c3d6cd3c96a1f973dc04ddff6164ec4b3a3f Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 19 Jan 2022 15:43:06 -0500 Subject: [PATCH 2/4] move Int subclass to jdaviz.core.custom_traitlets * and add test coverage to catch any breaks from upstream changes --- .../plugins/model_fitting/model_fitting.py | 20 ++----------- jdaviz/core/custom_traitlets.py | 26 +++++++++++++++++ jdaviz/core/tests/__init__.py | 0 jdaviz/core/tests/test_custom_traitlets.py | 28 +++++++++++++++++++ 4 files changed, 56 insertions(+), 18 deletions(-) create mode 100644 jdaviz/core/custom_traitlets.py create mode 100644 jdaviz/core/tests/__init__.py create mode 100644 jdaviz/core/tests/test_custom_traitlets.py diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index 6547c87543..be46036b50 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -8,7 +8,7 @@ SubsetUpdateMessage) from specutils import Spectrum1D, SpectralRegion from specutils.utils import QuantityModel -from traitlets import Any, Bool, Int, List, Unicode, observe +from traitlets import Any, Bool, List, Unicode, observe from glue.core.data import Data from glue.core.subset import Subset, RangeSubsetState, OrState, AndState from glue.core.link_helpers import LinkSame @@ -16,6 +16,7 @@ from jdaviz.core.events import AddDataMessage, RemoveDataMessage, SnackbarMessage from jdaviz.core.registries import tray_registry from jdaviz.core.template_mixin import TemplateMixin +from jdaviz.core.custom_traitlets import IntHandleEmpty from jdaviz.configs.default.plugins.model_fitting.fitting_backend import fit_model_to_spectrum from jdaviz.configs.default.plugins.model_fitting.initializers import (MODELS, initialize, @@ -24,23 +25,6 @@ __all__ = ['ModelFitting'] -class IntHandleEmpty(Int): - def __init__(self, *args, **kwargs): - self._empty_to_default = kwargs.pop('replace_with_default', False) - super().__init__(*args, **kwargs) - - def validate(self, obj, value): - if isinstance(value, str) and not len(value): - if self._empty_to_default: - # if the field is emptied, it will override with the default value - return self.default_value - # the value will remain as the empty string, likely will need to either - # couple this with form validation or handle the case where the value - # is an empty string once retrieved. - return value - return super().validate(obj, value) - - class _EmptyParam: def __init__(self, value, unit=None): self.value = value diff --git a/jdaviz/core/custom_traitlets.py b/jdaviz/core/custom_traitlets.py new file mode 100644 index 0000000000..5ef695fa4d --- /dev/null +++ b/jdaviz/core/custom_traitlets.py @@ -0,0 +1,26 @@ +from traitlets import Int, Float + + +class HandleEmptyMixin(object): + def __init__(self, *args, **kwargs): + self._empty_to_default = kwargs.pop('replace_with_default', False) + super().__init__(*args, **kwargs) + + def validate(self, obj, value): + if isinstance(value, str) and not len(value): + if self._empty_to_default: + # if the field is emptied, it will override with the default value + return self.default_value + # the value will remain as the empty string, likely will need to either + # couple this with form validation or handle the case where the value + # is an empty string once retrieved. + return value + return super().validate(obj, value) + + +class IntHandleEmpty(HandleEmptyMixin, Int): + pass + + +class FloatHandleEmpty(HandleEmptyMixin, Float): + pass diff --git a/jdaviz/core/tests/__init__.py b/jdaviz/core/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/jdaviz/core/tests/test_custom_traitlets.py b/jdaviz/core/tests/test_custom_traitlets.py new file mode 100644 index 0000000000..b7a1aa2505 --- /dev/null +++ b/jdaviz/core/tests/test_custom_traitlets.py @@ -0,0 +1,28 @@ +import pytest +from traitlets import HasTraits, TraitError + +from jdaviz.core.custom_traitlets import IntHandleEmpty, FloatHandleEmpty + + +class Foo(HasTraits): + int_handle_empty = IntHandleEmpty(0) + int_handle_empty_replace = IntHandleEmpty(0, replace_with_default=True) + float_handle_empty = FloatHandleEmpty() + + +def test_inthandleempty(): + foo = Foo() + + foo.int_handle_empty = 1 + foo.int_handle_empty = '' + assert foo.int_handle_empty == '' + with pytest.raises( + TraitError, + match="The 'int_handle_empty' trait of a Foo instance expected an int, not the str 'blah'."): # noqa + foo.int_handle_empty = 'blah' + + foo.int_handle_empty_replace = 1 + foo.int_handle_empty_replace = '' + assert foo.int_handle_empty_replace == 0 + + foo.float_handle_empty = 1.2 From 81e69a776a25b823de6e901e7f6b56b05c08c197 Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 19 Jan 2022 15:46:25 -0500 Subject: [PATCH 3/4] changelog entry --- CHANGES.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.rst b/CHANGES.rst index 97523dc3ee..5a9f89c3e3 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -26,6 +26,7 @@ Bug Fixes - Model plugin properly updates parameters after fit for compound models. [#1023] - Model plugin now respects fixed parameters when applying model to cube, and retains parameter units in that case. [#1026] +- Model plugin polynomial order now avoids traceback when clearing input. [#1041] Cubeviz ^^^^^^^ From f82c96377ab4fe7bb685511c5b4d7988653f340e Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Fri, 21 Jan 2022 09:01:45 -0500 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --- jdaviz/core/custom_traitlets.py | 6 +++--- jdaviz/core/tests/test_custom_traitlets.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/jdaviz/core/custom_traitlets.py b/jdaviz/core/custom_traitlets.py index 5ef695fa4d..34e8a861ba 100644 --- a/jdaviz/core/custom_traitlets.py +++ b/jdaviz/core/custom_traitlets.py @@ -1,7 +1,7 @@ from traitlets import Int, Float -class HandleEmptyMixin(object): +class HandleEmptyMixin: def __init__(self, *args, **kwargs): self._empty_to_default = kwargs.pop('replace_with_default', False) super().__init__(*args, **kwargs) @@ -9,9 +9,9 @@ def __init__(self, *args, **kwargs): def validate(self, obj, value): if isinstance(value, str) and not len(value): if self._empty_to_default: - # if the field is emptied, it will override with the default value + # If the field is emptied, it will override with the default value. return self.default_value - # the value will remain as the empty string, likely will need to either + # The value will remain as the empty string, likely will need to either # couple this with form validation or handle the case where the value # is an empty string once retrieved. return value diff --git a/jdaviz/core/tests/test_custom_traitlets.py b/jdaviz/core/tests/test_custom_traitlets.py index b7a1aa2505..f87f44f896 100644 --- a/jdaviz/core/tests/test_custom_traitlets.py +++ b/jdaviz/core/tests/test_custom_traitlets.py @@ -18,7 +18,7 @@ def test_inthandleempty(): assert foo.int_handle_empty == '' with pytest.raises( TraitError, - match="The 'int_handle_empty' trait of a Foo instance expected an int, not the str 'blah'."): # noqa + match=r"The 'int_handle_empty' trait of a Foo instance expected an int, not the str 'blah'\."): # noqa foo.int_handle_empty = 'blah' foo.int_handle_empty_replace = 1