glue-viz / glue-astronomy

Plugin to add astronomy-specific functionality to glue
https://glue-astronomy.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

Test failure with developer version of dependencies #42

Closed astrofrog closed 2 years ago

astrofrog commented 2 years ago

The following test failure exists in main when testing with e.g. py38-test-dev:


mode = 'wcs3d'

    @pytest.mark.parametrize('mode', ('wcs1d', 'wcs3d', 'lookup'))
    def test_from_spectrum1d(mode):

        if mode == 'wcs3d':
            # This test is intended to be run with the version of Spectrum1D based
            # on NDCube 2.0
            pytest.importorskip("ndcube", minversion="1.99")

            # Set up simple spatial+spectral WCS
            wcs = WCS(naxis=3)
            wcs.wcs.ctype = ['RA---TAN', 'DEC--TAN', 'FREQ']
            wcs.wcs.set()
            flux = np.ones((4, 4, 5))*u.Unit('Jy')
            uncertainty = VarianceUncertainty(np.square(flux*0.1))
            mask = np.zeros((4, 4, 5))
            kwargs = {'wcs': wcs, 'uncertainty': uncertainty, 'mask': mask}
        else:
            flux = [2, 3, 4, 5] * u.Jy
            uncertainty = VarianceUncertainty([0.1, 0.1, 0.1, 0.1] * u.Jy**2)
            mask = [False, False, False, False]
            if mode == 'wcs1d':
                wcs = WCS(naxis=1)
                wcs.wcs.ctype = ['FREQ']
                wcs.wcs.set()
                kwargs = {'wcs': wcs, 'uncertainty': uncertainty, 'mask': mask}
            else:
                kwargs = {'spectral_axis': [1, 2, 3, 4] * u.Hz,
                          'uncertainty': uncertainty, 'mask': mask}

        spec = Spectrum1D(flux, **kwargs)

        data_collection = DataCollection()

        data_collection['spectrum'] = spec

        data = data_collection['spectrum']

        assert isinstance(data, Data)
        assert len(data.main_components) == 3
        assert data.main_components[0].label == 'flux'
>       assert_allclose(data['flux'], flux.value)
E       AssertionError: 
E       Not equal to tolerance rtol=1e-07, atol=0
E       
E       (shapes (4, 5, 4), (4, 4, 5) mismatch)
E        x: array([[[1., 1., 1., 1.],
E               [1., 1., 1., 1.],
E               [1., 1., 1., 1.],...
E        y: array([[[1., 1., 1., 1., 1.],
E               [1., 1., 1., 1., 1.],
E               [1., 1., 1., 1., 1.],...

glue_astronomy/translators/tests/test_spectrum1d.py:167: AssertionError

@rosteen - since you added this test, can you comment on why this might be failing?

pllim commented 2 years ago

Input has (4, 4, 5) shape but somehow data['flux'] has (4, 5, 4) shape. 🀯

astrofrog commented 2 years ago

Specutils does horrible things to input arrays πŸ˜‚

astrofrog commented 2 years ago

Actually I think the test is wrong since we deliberately re-order the axes between Spectrum1D and glue data objects, so should be a simple fix. I'll do it.

pllim commented 2 years ago

But why is it not failing for other jobs?

astrofrog commented 2 years ago

Because that test is conditional on dev version of dependencies:

        pytest.importorskip("ndcube", minversion="1.99")
rosteen commented 2 years ago

Sorry for not commenting on this sooner. It started failing on dev because https://github.com/astropy/specutils/pull/822 finally got merged, changing the order that the axes end up after automatically moving the spectral axis to be last. Acknowledged that this is...not ideal πŸ˜…

Previous to that fix on dev the WCS and actual data were getting misaligned when automatically reordering the axes for the spectral axis to be last. So "less than ideal" is better than "actually just wrong".

astrofrog commented 2 years ago

Actually I believe that astropy/specutils#822 actually fixed the test. I was seeing failures now because astropy dropped Python 3.7 support in main so I've bumped the Python version used to do the dev CI - let's see if that works now.

astrofrog commented 2 years ago

Yes, the CI is all fine now. I've released glue-astronomy v0.3!

pllim commented 2 years ago

Merci! Are you going to update pin on jdaviz too? πŸ˜‰

astrofrog commented 2 years ago

Yes in 733. Going to push commits soon.