From c29f7719b6b238ca9e88a570018c9242b079428a Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 24 May 2023 13:03:28 -0400 Subject: [PATCH 1/6] model component compatibility based on display units --- .../plugins/model_fitting/model_fitting.py | 21 ++++++++++++++++++- .../plugins/model_fitting/model_fitting.vue | 5 ++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index 1620db5df7..9e27d612bc 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 @@ from traitlets import Bool, List, Unicode, observe from glue.core.data import Data -from jdaviz.core.events import SnackbarMessage +from jdaviz.core.events import SnackbarMessage, GlobalDisplayUnitChanged from jdaviz.core.registries import tray_registry from jdaviz.core.template_mixin import (PluginTemplateMixin, SelectPluginComponent, @@ -169,6 +169,9 @@ def __init__(self, *args, **kwargs): # set the filter on the viewer options self._update_viewer_filters() + self.hub.subscribe(self, GlobalDisplayUnitChanged, + handler=self._on_global_display_unit_changed) + @property def user_api(self): expose = ['dataset'] @@ -336,6 +339,7 @@ def _dataset_selected_changed(self, event=None): # (won't affect calculations because these locations are masked) selected_spec.flux[np.isnan(selected_spec.flux)] = 0.0 + # TODO: can we simplify this logic? self._units["x"] = str( selected_spec.spectral_axis.unit) self._units["y"] = str( @@ -503,8 +507,22 @@ def _initialize_model_component(self, model_comp, comp_label, poly_order=None): self._initialized_models[comp_label] = initialized_model new_model["Initialized"] = True + new_model["initialized_display_units"] = self._units + + new_model["compat_display_units"] = True # always compatible at time of creation return new_model + def _on_global_display_unit_changed(self, msg): + disp_physical_type = u.Unit(msg.unit).physical_type + axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis) + for model_index, comp_model in enumerate(self.component_models): + comp_unit = u.Unit(comp_model["initialized_display_units"][axis]) + compat = comp_unit.physical_type == disp_physical_type + self.component_models[model_index]["compat_display_units"] = compat + + # length hasn't changed, so we need to force the traitlet to update + self.send_state("component_models") + def remove_model_component(self, model_component_label): """ Remove an existing model component. @@ -663,6 +681,7 @@ def _model_equation_changed(self, event): if len(self.model_equation) == 0: self.model_equation_invalid_msg = 'model equation is required' return + # TODO: split and check that each component exists and is currently valid with display units self.model_equation_invalid_msg = '' @observe("dataset_selected", "dataset_items", "cube_fit") diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue index e42107cf78..b59d0c42b2 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue @@ -127,7 +127,7 @@ - + + + this component is disabled because the units are incompatible with currently set display units + From 4ecdae7b4f89eeb85db7234f549f8ef14450a5ee Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Thu, 25 May 2023 10:49:34 -0400 Subject: [PATCH 2/6] equation validity based on display units --- .../plugins/model_fitting/model_fitting.py | 56 +++++++++++++++++-- .../plugins/model_fitting/model_fitting.vue | 20 +++---- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index 9e27d612bc..89650d3610 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -60,6 +60,7 @@ class ModelFitting(PluginTemplateMixin, DatasetSelectMixin, * :meth:`create_model_component` * :meth:`remove_model_component` * :meth:`model_components` + * :meth:`valid_model_components` * :meth:`get_model_component` * :meth:`set_model_component` * :meth:`reestimate_model_parameters` @@ -178,7 +179,8 @@ def user_api(self): if self.config == "cubeviz": expose += ['spatial_subset'] expose += ['spectral_subset', 'model_component', 'poly_order', 'model_component_label', - 'model_components', 'create_model_component', 'remove_model_component', + 'model_components', 'valid_model_components', + 'create_model_component', 'remove_model_component', 'get_model_component', 'set_model_component', 'reestimate_model_parameters', 'equation', 'equation_components', 'add_results', 'residuals_calculate', 'residuals'] @@ -507,14 +509,19 @@ def _initialize_model_component(self, model_comp, comp_label, poly_order=None): self._initialized_models[comp_label] = initialized_model new_model["Initialized"] = True - new_model["initialized_display_units"] = self._units + new_model["initialized_display_units"] = self._units.copy() new_model["compat_display_units"] = True # always compatible at time of creation return new_model def _on_global_display_unit_changed(self, msg): - disp_physical_type = u.Unit(msg.unit).physical_type axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis) + + # update internal tracking of current units + self._units[axis] = str(msg.unit) + + # update validity of model components + disp_physical_type = msg.unit.physical_type for model_index, comp_model in enumerate(self.component_models): comp_unit = u.Unit(comp_model["initialized_display_units"][axis]) compat = comp_unit.physical_type == disp_physical_type @@ -522,6 +529,7 @@ def _on_global_display_unit_changed(self, msg): # length hasn't changed, so we need to force the traitlet to update self.send_state("component_models") + self._check_model_equation_invalid() def remove_model_component(self, model_component_label): """ @@ -662,6 +670,13 @@ def model_components(self): """ return [x["id"] for x in self.component_models] + @property + def valid_model_components(self): + """ + List of the labels of existing valid (due to display units) model components + """ + return [x["id"] for x in self.component_models if x["compat_display_units"]] + @property def equation_components(self): """ @@ -676,12 +691,41 @@ def vue_remove_model(self, event): self.remove_model_component(event) @observe('model_equation') - def _model_equation_changed(self, event): + def _check_model_equation_invalid(self, event=None): # Length is a dummy check to test the infrastructure if len(self.model_equation) == 0: - self.model_equation_invalid_msg = 'model equation is required' + self.model_equation_invalid_msg = 'model equation is required.' + return + if '' in self.equation_components: + # includes an operator without a variable (ex: 'C+') + self.model_equation_invalid_msg = 'incomplete equation.' + return + components_not_existing = [comp for comp in self.equation_components + if comp not in self.model_components] + if len(components_not_existing): + if len(components_not_existing) == 1: + msg = "is not an existing model component." + else: + msg = "are not existing model components." + self.model_equation_invalid_msg = f'{", ".join(components_not_existing)} {msg}' + return + components_not_valid = [comp for comp in self.equation_components + if comp not in self.valid_model_components] + if len(components_not_valid): + if len(components_not_valid) == 1: + msg = ("is currently disabled because it has" + " incompatible units with the current display units." + " Remove the component from the equation," + " recreate the model component to have the new units," + " or revert the display units.") + else: + msg = ("are currently disabled because they have" + " incompatible units with the current display units." + " Remove the components from the equation," + " recreate the model components to have the new units," + " or revert the display units.") + self.model_equation_invalid_msg = f'{", ".join(components_not_valid)} {msg}' return - # TODO: split and check that each component exists and is currently valid with display units self.model_equation_invalid_msg = '' @observe("dataset_selected", "dataset_items", "cube_fit") diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue index b59d0c42b2..640dee2db0 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue @@ -127,13 +127,16 @@ - - - {{ item.id }} model component not in equation + + + + {{ item.id }} is inconsistent with the current display units so cannot be used in the model equation. + + + + + {{ item.id }} model component not in equation + - - this component is disabled because the units are incompatible with currently set display units - From 8cd968ca55dcb99d49e0d55b3fb6850a17c4276b Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Thu, 25 May 2023 11:14:18 -0400 Subject: [PATCH 3/6] allow re-estimating model parameters to update compat checks --- jdaviz/components/tooltip.vue | 2 +- .../plugins/model_fitting/model_fitting.py | 34 +++++++++++++------ .../plugins/model_fitting/model_fitting.vue | 23 ++++++++++++- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/jdaviz/components/tooltip.vue b/jdaviz/components/tooltip.vue index 1bd8f37c1f..91c81e1e5c 100644 --- a/jdaviz/components/tooltip.vue +++ b/jdaviz/components/tooltip.vue @@ -76,7 +76,7 @@ const tooltips = { 'plugin-plot-options-mixed-state': 'Current values are mixed, click to sync at shown value', 'plugin-model-fitting-add-model': 'Create model component', 'plugin-model-fitting-param-fixed': 'Check the box to freeze parameter value', - 'plugin-model-fitting-reestimate-all': 'Re-estimate initial values based on the current data/subset selection for all free parameters', + 'plugin-model-fitting-reestimate-all': 'Re-estimate initial values based on the current data/subset selection for all free parameters based on current display units', 'plugin-model-fitting-reestimate': 'Re-estimate initial values based on the current data/subset selection for all free parameters in this component', 'plugin-unit-conversion-apply': 'Apply unit conversion', 'plugin-line-lists-load': 'Load list into "Loaded Lines" section of plugin', diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index 89650d3610..dcf3576704 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -514,23 +514,33 @@ def _initialize_model_component(self, model_comp, comp_label, poly_order=None): new_model["compat_display_units"] = True # always compatible at time of creation return new_model - def _on_global_display_unit_changed(self, msg): - axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis) + def _check_model_component_compat(self, axes=['x', 'y'], display_units=None): + if display_units is None: + display_units = [u.Unit(self._units[ax]) for ax in axes] - # update internal tracking of current units - self._units[axis] = str(msg.unit) + disp_physical_types = [unit.physical_type for unit in display_units] - # update validity of model components - disp_physical_type = msg.unit.physical_type for model_index, comp_model in enumerate(self.component_models): - comp_unit = u.Unit(comp_model["initialized_display_units"][axis]) - compat = comp_unit.physical_type == disp_physical_type + compat = True + for ax, ax_physical_type in zip(axes, disp_physical_types): + comp_unit = u.Unit(comp_model["initialized_display_units"][ax]) + compat = comp_unit.physical_type == ax_physical_type + if not compat: + break self.component_models[model_index]["compat_display_units"] = compat # length hasn't changed, so we need to force the traitlet to update self.send_state("component_models") self._check_model_equation_invalid() + def _on_global_display_unit_changed(self, msg): + axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis) + + # update internal tracking of current units + self._units[axis] = str(msg.unit) + + self._check_model_component_compat([axis], [msg.unit]) + def remove_model_component(self, model_component_label): """ Remove an existing model component. @@ -660,6 +670,9 @@ def reestimate_model_parameters(self, model_component_label=None): # length hasn't changed, so we need to force the traitlet to update self.send_state("component_models") + # model units may have changed, need to re-check their compatibility with display units + self._check_model_component_compat() + # return user-friendly info on revised model return self.get_model_component(model_component_label) @@ -700,6 +713,7 @@ def _check_model_equation_invalid(self, event=None): # includes an operator without a variable (ex: 'C+') self.model_equation_invalid_msg = 'incomplete equation.' return + components_not_existing = [comp for comp in self.equation_components if comp not in self.model_components] if len(components_not_existing): @@ -716,13 +730,13 @@ def _check_model_equation_invalid(self, event=None): msg = ("is currently disabled because it has" " incompatible units with the current display units." " Remove the component from the equation," - " recreate the model component to have the new units," + " re-estimate its free parameters to use the new units" " or revert the display units.") else: msg = ("are currently disabled because they have" " incompatible units with the current display units." " Remove the components from the equation," - " recreate the model components to have the new units," + " re-estimate their free parameters to use the new units" " or revert the display units.") self.model_equation_invalid_msg = f'{", ".join(components_not_valid)} {msg}' return diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue index 640dee2db0..cd21defd3f 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue @@ -131,6 +131,26 @@ {{ item.id }} is inconsistent with the current display units so cannot be used in the model equation. + Create a new model component or re-estimate the free parameters based on the current display units. + + + + mdi-restart + Re-estimate free parameters + + + @@ -138,7 +158,8 @@ {{ item.id }} model component not in equation - From 731ba210d53913c98f771d7d0e105aeb1f945e0f Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 31 May 2023 08:25:12 -0400 Subject: [PATCH 4/6] test coverage --- .../plugins/model_fitting/model_fitting.py | 2 + .../model_fitting/tests/test_fitting.py | 53 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index dcf3576704..1604f72c4d 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -784,6 +784,8 @@ def calculate_fit(self, add_data=True): if not self.spectral_subset_valid: valid, spec_range, subset_range = self._check_dataset_spectral_subset_valid(return_ranges=True) # noqa raise ValueError(f"spectral subset '{self.spectral_subset.selected}' {subset_range} is outside data range of '{self.dataset.selected}' {spec_range}") # noqa + if len(self.model_equation_invalid_msg): + raise ValueError(f"model equation is invalid: {self.model_equation_invalid_msg}") if self.cube_fit: ret = self._fit_model_to_cube(add_data=add_data) diff --git a/jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py b/jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py index 41d418a0ec..22c059d64a 100644 --- a/jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/tests/test_fitting.py @@ -327,3 +327,56 @@ def test_results_table(specviz_helper, spectrum1d): 'G:mean_1:fixed', 'G:mean_1:std', 'G:stddev_1', 'G:stddev_1:unit', 'G:stddev_1:fixed', 'G:stddev_1:std'] + + +def test_equation_validation(specviz_helper, spectrum1d): + data_label = 'test' + specviz_helper.load_data(spectrum1d, data_label=data_label) + + mf = specviz_helper.plugins['Model Fitting'] + mf.create_model_component('Const1D') + mf.create_model_component('Linear1D') + + assert mf.equation == 'C+L' + assert mf._obj.model_equation_invalid_msg == '' + + mf.equation = 'L+' + assert mf._obj.model_equation_invalid_msg == 'incomplete equation.' + + mf.equation = 'L+C' + assert mf._obj.model_equation_invalid_msg == '' + + mf.equation = 'L+CC' + assert mf._obj.model_equation_invalid_msg == 'CC is not an existing model component.' + + mf.equation = 'L+CC+DD' + assert mf._obj.model_equation_invalid_msg == 'CC, DD are not existing model components.' + + mf.equation = '' + assert mf._obj.model_equation_invalid_msg == 'model equation is required.' + + +@pytest.mark.filterwarnings(r"ignore:Model is linear in parameters.*") +@pytest.mark.filterwarnings(r"ignore:The fit may be unsuccessful.*") +def test_incompatible_units(specviz_helper, spectrum1d): + data_label = 'test' + specviz_helper.load_data(spectrum1d, data_label=data_label) + + mf = specviz_helper.plugins['Model Fitting'] + mf.create_model_component('Linear1D') + + mf.add_results.label = 'model native units' + mf.calculate_fit(add_data=True) + + uc = specviz_helper.plugins['Unit Conversion'] + assert uc.spectral_unit.selected == "Angstrom" + uc.spectral_unit = u.Hz + + assert 'L is currently disabled' in mf._obj.model_equation_invalid_msg + mf.add_results.label = 'frequency units' + with pytest.raises(ValueError, match=r"model equation is invalid.*"): + mf.calculate_fit(add_data=True) + + mf.reestimate_model_parameters() + assert mf._obj.model_equation_invalid_msg == '' + mf.calculate_fit(add_data=True) From 271d7247dc6b82391390d983349281c74e02983f Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Wed, 31 May 2023 16:02:58 -0400 Subject: [PATCH 5/6] get_data(use_display_units) to pass spectral_density equivalency --- jdaviz/core/helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jdaviz/core/helpers.py b/jdaviz/core/helpers.py index f70e582740..40dbd87740 100644 --- a/jdaviz/core/helpers.py +++ b/jdaviz/core/helpers.py @@ -431,7 +431,8 @@ def _handle_display_units(data, use_display_units): new_uncert = data.uncertainty.__class__(data.uncertainty.quantity.to(flux_unit)) if data.uncertainty is not None else None # noqa data = Spectrum1D(spectral_axis=data.spectral_axis.to(spectral_unit, u.spectral()), - flux=data.flux.to(flux_unit), + flux=data.flux.to(flux_unit, + u.spectral_density(data.spectral_axis)), uncertainty=new_uncert) else: # pragma: nocover raise NotImplementedError(f"converting {data.__class__.__name__} to display units is not supported") # noqa From 1aaf310b3e2461ca1a49d787a3b1c631d4d980b1 Mon Sep 17 00:00:00 2001 From: Kyle Conroy Date: Thu, 1 Jun 2023 08:58:16 -0400 Subject: [PATCH 6/6] handle whitespace in model equation --- jdaviz/configs/default/plugins/model_fitting/model_fitting.py | 2 +- jdaviz/configs/default/plugins/model_fitting/model_fitting.vue | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py index 1604f72c4d..e7ccc53395 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.py +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.py @@ -695,7 +695,7 @@ def equation_components(self): """ List of the labels of model components in the current equation """ - return re.split('[+*/-]', self.equation.value) + return re.split(r'[+*/-]', self.equation.value.replace(' ', '')) def vue_add_model(self, event): self.create_model_component() diff --git a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue index cd21defd3f..8da1c00bc0 100644 --- a/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue +++ b/jdaviz/configs/default/plugins/model_fitting/model_fitting.vue @@ -314,7 +314,7 @@ }, methods: { componentInEquation(componentId) { - return this.model_equation.split(/[+*\/-]/).indexOf(componentId) !== -1 + return this.model_equation.replace(/\s/g, '').split(/[+*\/-]/).indexOf(componentId) !== -1 }, roundUncertainty(uncertainty) { return uncertainty.toPrecision(2)