Skip to content

Conversation

@mariobuikhuizen
Copy link
Collaborator

Description

This pull request adds handles on subset selection
Screenshot 2025-12-02 at 16 02 12

Needs glue-viz/glue-jupyter#498

Change log entry

  • Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
    list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
    should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • If new remote data is added that uses MAST, is the URI added to the cache-download.yml workflow?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@MatthewPortman
Copy link
Contributor

MatthewPortman commented Dec 3, 2025

I'm not sure if this is on our end due to but I wanted to make note of it here --
I'm getting a UnitConversionError when attempting to load any kind of data into deconfigged. Other configs seem to work though so I'm still able to test functionality. When I use glue-viz/glue-jupyter#498, it occurs with both this PR and on main --

...
File [~/jdaviz_test/jdaviz/core/loaders/importers/image/image.py:324](http://localhost:8889/~/jdaviz_test/jdaviz/core/loaders/importers/image/image.py#line=323), in ImageImporter.__call__(self)
    321 if self.gwcs_to_fits_sip:
    322     output = self._glue_data_wcs_to_fits(output)
--> 324 self.add_to_data_collection(output, data_label, data_hash=ext_item.get('data_hash'),
    325                             parent=parent_data_label if parent_data_label != data_label else None,  # noqa
    326                             cls=CCDData)

File [~/jdaviz_test/jdaviz/core/loaders/importers/importer.py:322](http://localhost:8889/~/jdaviz_test/jdaviz/core/loaders/importers/importer.py#line=321), in BaseImporterToDataCollection.add_to_data_collection(self, data, data_label, data_hash, parent, viewer_select, cls)
    317 self.app._on_new_viewer(NewViewerMessage(viewer_cls, data=None, sender=self.app),
    318                         vid=viewer_label,
    319                         name=viewer_label,
    320                         open_data_menu_if_empty=False)
    321 viewer = self.app._jdaviz_helper.viewers.get(viewer_label)
--> 322 viewer.data_menu.add_data(data_label)
    324 # default to selecting this new viewer for next import
    325 viewer_select.create_new.selected = ''

File [~/jdaviz_test/jdaviz/configs/default/plugins/data_menu/data_menu.py:579](http://localhost:8889/~/jdaviz_test/jdaviz/configs/default/plugins/data_menu/data_menu.py#line=578), in DataMenu.add_data(self, *data_labels)
    577     raise ValueError(f"Data labels {unavailable} not able to be loaded into '{self.viewer_id}'.  Must be one of: {self.dataset.choices}")  # noqa
    578 for data_label in data_labels:
--> 579     self.app.add_data_to_viewer(self.viewer_id, data_label)

File [~/jdaviz_test/jdaviz/app.py:1889](http://localhost:8889/~/jdaviz_test/jdaviz/app.py#line=1888), in Application.add_data_to_viewer(self, viewer_reference, data_label, visible, clear_other_data)
   1886 if viewer_item is None:
   1887     raise ValueError(f"Could not identify viewer with reference {viewer_reference}")
-> 1889 self.set_data_visibility(viewer_item, data_label, visible=visible, replace=clear_other_data)

File [~/jdaviz_test/jdaviz/app.py:2551](http://localhost:8889/~/jdaviz_test/jdaviz/app.py#line=2550), in Application.set_data_visibility(self, viewer_reference, data_label, visible, replace)
   2546     viewer.set_plot_axes()
   2548     add_data_message = AddDataMessage(data, viewer,
   2549                                       viewer_id=viewer_id,
   2550                                       sender=self)
-> 2551     self.hub.broadcast(add_data_message)
   2553 assoc_children = self._get_assoc_data_children(data_label)
   2555 # set visibility state of all applicable layers

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/core/hub.py:225](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/core/hub.py#line=224), in Hub.broadcast(self, message)
    223 logging.getLogger(__name__).info("Broadcasting %s", message)
    224 for subscriber, handler in self._find_handlers(message):
--> 225     handler(message)

File [~/jdaviz_test/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py:363](http://localhost:8889/~/jdaviz_test/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py#line=362), in UnitConversion._on_add_data_to_viewer(self, msg)
    360             self.angle_unit.selected = ''
    362 if self.angle_unit:
--> 363     self._handle_attribute_display_unit(self.sb_unit_selected, layers=layers)
    364 self._clear_cache('image_layers')

File [~/jdaviz_test/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py:507](http://localhost:8889/~/jdaviz_test/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py#line=506), in UnitConversion._handle_attribute_display_unit(self, attr_unit, layers)
    505     ctx = nullcontext()
    506 with ctx:
--> 507     layer.state.attribute_display_unit = _valid_glue_display_unit(
    508         attr_unit, layer, 'attribute')

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py:313](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py#line=312), in HasCallbackProperties.__setattr__(self, attribute, value)
    311 if is_callback:
    312     previous_value = getattr(self, attribute, None)
--> 313 super(HasCallbackProperties, self).__setattr__(attribute, value)
    314 if is_callback and value != previous_value:
    315     self._notify_global(**{attribute: value})

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/selection.py:41](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/selection.py#line=40), in SelectionCallbackProperty.__set__(self, instance, value)
     39         if not any(value is x for x in choices):
     40             raise ValueError('value {0} is not in valid choices: {1}'.format(value, choices))
---> 41 super(SelectionCallbackProperty, self).__set__(instance, value)

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py:83](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py#line=82), in CallbackProperty.__set__(self, instance, value)
     81 new = self.__get__(instance)
     82 if old != new:
---> 83     self.notify(instance, old, new)

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/utils/matplotlib.py:172](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/utils/matplotlib.py#line=171), in defer_draw.<locals>.wrapper(*args, **kwargs)
    168 @wraps(func)
    169 def wrapper(*args, **kwargs):
    171     if len(DEFER_DRAW_BACKENDS) == 0:
--> 172         return func(*args, **kwargs)
    174     # Don't recursively defer draws. We just check the first draw_idle
    175     # method since all should be modified in sync.
    176     if isinstance(DEFER_DRAW_BACKENDS[0].draw_idle, DeferredMethod):

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/viewers/matplotlib/state.py:38](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/viewers/matplotlib/state.py#line=37), in DeferredDrawSelectionCallbackProperty.notify(self, *args, **kwargs)
     36 @defer_draw
     37 def notify(self, *args, **kwargs):
---> 38     super(DeferredDrawSelectionCallbackProperty, self).notify(*args, **kwargs)

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py:134](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/echo/core.py#line=133), in CallbackProperty.notify(self, instance, old, new)
    132     cback(new)
    133 for cback in self._2arg_callbacks.get(instance, []):
--> 134     cback(old, new)

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue_jupyter/bqplot/image/state.py:96](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue_jupyter/bqplot/image/state.py#line=95), in BqplotImageLayerState._convert_units_c_limits(self, old_unit, new_unit)
     90 converter = UnitConverter()
     92 limits_native = converter.to_native(self.layer,
     93                                     self.attribute, limits,
     94                                     old_unit)
---> 96 limits_new = converter.to_unit(self.layer,
     97                                self.attribute, limits_native,
     98                                new_unit)
    100 with delay_callback(self, 'c_min', 'c_max', 'levels'):
    101     self.c_min, self.c_max = sorted(limits_new[:2])

File [/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/core/units.py:33](http://localhost:8889/opt/miniconda3/envs/jdaviz-test/lib/python3.13/site-packages/glue/core/units.py#line=32), in UnitConverter.to_unit(self, data, cid, values, target_units)
     31     return values
     32 else:
---> 33     return self.converter_helper.to_unit(data, cid, values, original_units, target_units)

File [~/jdaviz_test/jdaviz/app.py:129](http://localhost:8889/~/jdaviz_test/jdaviz/app.py#line=128), in UnitConverterWithSpectral.to_unit(self, data, cid, values, original_units, target_units)
    125     return values
    126 elif (data.meta.get('_importer') == 'ImageImporter' and
    127       u.Unit(data.get_component(cid).units).physical_type == 'surface brightness'):
    128     # handle surface brightness units in image-like data
--> 129     return (values * u.Unit(original_units)).to_value(target_units)
    130 elif cid.label in ("flux"):
    131     try:

File [~/astropy/astropy/units/quantity.py:972](http://localhost:8889/~/astropy/astropy/units/quantity.py#line=971), in Quantity.to_value(self, unit, equivalencies)
    969     scale = self.unit._to(unit)
    970 except Exception:
    971     # Short-cut failed; try default (maybe equivalencies help).
--> 972     value = self._to_value(unit, equivalencies)
    973 else:
    974     value = self.view(np.ndarray)

File [~/astropy/astropy/units/quantity.py:878](http://localhost:8889/~/astropy/astropy/units/quantity.py#line=877), in Quantity._to_value(self, unit, equivalencies)
    875     equivalencies = self._equivalencies
    876 if not self.dtype.names or isinstance(self.unit, StructuredUnit):
    877     # Standard path, let unit to do work.
--> 878     return self.unit.to(
    879         unit, self.view(np.ndarray), equivalencies=equivalencies
    880     )
    882 else:
    883     # The .to() method of a simple unit cannot convert a structured
    884     # dtype, so we work around it, by recursing.
    885     # TODO: deprecate this?
    886     # Convert simple to Structured on initialization?
    887     result = np.empty_like(self.view(np.ndarray))

File [~/astropy/astropy/units/core.py:660](http://localhost:8889/~/astropy/astropy/units/core.py#line=659), in UnitBase.to(self, other, value, equivalencies)
    658     return UNITY
    659 else:
--> 660     return self.get_converter(Unit(other), equivalencies)(value)

File [~/astropy/astropy/units/core.py:589](http://localhost:8889/~/astropy/astropy/units/core.py#line=588), in UnitBase.get_converter(self, other, equivalencies)
    586             else:
    587                 return lambda v: b(converter(v))
--> 589 raise exc

File [~/astropy/astropy/units/core.py:572](http://localhost:8889/~/astropy/astropy/units/core.py#line=571), in UnitBase.get_converter(self, other, equivalencies)
    570 # if that doesn't work, maybe we can do it with equivalencies?
    571 try:
--> 572     return self._apply_equivalencies(
    573         self, other, self._normalize_equivalencies(equivalencies)
    574     )
    575 except UnitsError as exc:
    576     # Last hope: maybe other knows how to do it?
    577     # We assume the equivalencies have the unit itself as first item.
    578     # TODO: maybe better for other to have a `_back_converter` method?
    579     if hasattr(other, "equivalencies"):

File [~/astropy/astropy/units/core.py:523](http://localhost:8889/~/astropy/astropy/units/core.py#line=522), in UnitBase._apply_equivalencies(self, unit, other, equivalencies)
    520 unit_str = get_err_str(unit)
    521 other_str = get_err_str(other)
--> 523 raise UnitConversionError(f"{unit_str} and {other_str} are not convertible")

UnitConversionError: 'MJy / sr' (surface brightness) and 'Jy' (spectral flux density) are not convertible```

@kecnry
Copy link
Member

kecnry commented Dec 3, 2025

very unlikely related to this and you said it appears on main - can you please open a ticket or issue?

@dhomeier
Copy link
Collaborator

dhomeier commented Dec 9, 2025

That exception's from astropy 7.1.1 if I'm not mistaken, but still could be an error in glue-jupyter that just silently slipped through before (or incorrect target_units in jdaviz).

@MatthewPortman
Copy link
Contributor

That exception's from astropy 7.1.1 if I'm not mistaken, but still could be an error in glue-jupyter that just silently slipped through before (or incorrect target_units in jdaviz).

I tried it in a different environment and didn't see the same issue... I'm not sure what caused it (Astropy was up to date) but given that it works elsewhere, I'm no longer worried.

Copy link
Contributor

@MatthewPortman MatthewPortman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I will approve once the upstream handle functionality is merged

@rosteen rosteen modified the milestones: 4.5, 4.6 Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants