Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

temporarily avoid conflicts introduced by recent glue releases #109

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Apr 18, 2024

with glue >= 1.19.0, we are getting the following traceback when loading data:

../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/viewers/image/state.py:607: in _update_attribute_display_unit_choices
    self.attribute_display_unit = component.units
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/echo/core.py:263: in __setattr__
    self._notify_global(**{attribute: value})
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/utils/matplotlib.py:170: in wrapper
    return func(*args, **kwargs)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/viewers/matplotlib/state.py:319: in _notify_global
    super(MatplotlibLayerState, self)._notify_global(*args, **kwargs)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/echo/core.py:258: in _notify_global
    callback(**kwargs)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/core/state_objects.py:206: in _update_values
    self.update_values(**properties)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/core/state_objects.py:346: in update_values
    limits_native = converter.to_native(self.data,
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/core/units.py:40: in to_native
    return self.converter_helper.to_unit(data, cid, values, original_units, target_units)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/jdaviz/app.py:98: in to_unit
    spec = data.get_object(cls=Spectrum1D)
../../.tox/py310-test-alldeps-cov/lib/python3.10/site-packages/glue/core/data.py:298: in get_object
    return handler.to_object(self, **kwargs)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <glue_astronomy.translators.spectrum1d.Specutils1DHandler object at 0x7fd9cf37ccd0>
data_or_subset = Data (label: contents[pdcsap_flux]), attribute = None
statistic = None

    def to_object(self, data_or_subset, attribute=None, statistic='mean'):
        """
        Convert a glue Data object to a Spectrum1D object.
    
        Parameters
        ----------
        data_or_subset : `glue.core.data.Data` or `glue.core.subset.Subset`
            The data to convert to a Spectrum1D object
        attribute : `glue.core.component_id.ComponentID`
            The attribute to use for the Spectrum1D data
        statistic : {'minimum', 'maximum', 'mean', 'median', 'sum', 'percentile'}
            The statistic to use to collapse the dataset
        """
    
        if isinstance(data_or_subset, Subset):
            data = data_or_subset.data
            subset_state = data_or_subset.subset_state
        else:
            data = data_or_subset
            subset_state = None
    
        if data.ndim < 2 and statistic is not None:
            statistic = None
    
        if statistic is None and isinstance(data.coords, BaseHighLevelWCS):
    
            if isinstance(data.coords, PaddedSpectrumWCS):
                kwargs = {'wcs': data.coords.spectral_wcs}
            else:
                kwargs = {'wcs': data.coords}
    
        elif statistic is not None:
    
            if isinstance(data.coords, PaddedSpectrumWCS):
                spec_axis = 0
                axes = tuple(range(0, data.ndim-1))
                kwargs = {'wcs': data.coords.spectral_wcs}
            elif isinstance(data.coords, WCS):
    
                # Find spectral axis
                spec_axis = data.coords.naxis - 1 - data.coords.wcs.spec
    
                # Find non-spectral axes
                axes = tuple(i for i in range(data.ndim) if i != spec_axis)
    
                kwargs = {'wcs': data.coords.sub([WCSSUB_SPECTRAL])}
    
            else:
                raise ValueError('Can only use statistic= if the Data object has a FITS WCS')
    
        elif isinstance(data.coords, SpectralCoordinates):
    
            kwargs = {'spectral_axis': data.coords.spectral_axis}
    
        else:
>           raise TypeError('data.coords should be an instance of WCS '
                            'or SpectralCoordinates')
E           TypeError: data.coords should be an instance of WCS or SpectralCoordinates

@kecnry kecnry marked this pull request as ready for review April 18, 2024 13:51
Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.71%. Comparing base (7b78df5) to head (a18f23d).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
- Coverage   93.92%   92.71%   -1.22%     
==========================================
  Files          37       39       +2     
  Lines        1598     1881     +283     
==========================================
+ Hits         1501     1744     +243     
- Misses         97      137      +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kecnry kecnry merged commit 3ec3916 into spacetelescope:main Apr 18, 2024
9 of 11 checks passed
kecnry added a commit that referenced this pull request Apr 18, 2024
@kecnry kecnry deleted the max-pin-glue branch April 18, 2024 13:56
@dhomeier
Copy link

Hmm, in
https://github.com/spacetelescope/lcviz/actions/runs/8738756878/job/23978473729?pr=107#step:5:407
your units converter is calling a glue_astronomy converter for Spectrum1D, although in
https://github.com/spacetelescope/lcviz/actions/runs/8738756878/job/23978473729?pr=107#step:5:331
this is loading a Kepler TPF; so it looks like somewhere along the way a wrong component_id is set.

@kecnry
Copy link
Member Author

kecnry commented Apr 19, 2024

Now with spacetelescope/jdaviz#2820 upstream, I think this should go away once jdaviz 3.9.1 is released. But when we want to get display unit support in lcviz, we'll need to modify the logic upstream to be more general and not just assume that flux means Spectrum1D.

kecnry added a commit that referenced this pull request Apr 19, 2024
kecnry added a commit that referenced this pull request Apr 19, 2024
kecnry added a commit that referenced this pull request Apr 19, 2024
@kecnry
Copy link
Member Author

kecnry commented Apr 19, 2024

#110 reverts the max-pin by bypassing jdaviz's custom unit converter and implementing a barebones one for lcviz, but that is failing when glue requests converting between days and electron/s. See #110 (comment)

kecnry added a commit to kecnry/lcviz that referenced this pull request May 24, 2024
kecnry added a commit to kecnry/lcviz that referenced this pull request Jun 9, 2024
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.

2 participants