Bugfix: stringy physical unit types #709

keflavich · 2021-05-01T18:39:24Z

Running with the latest astropy version (a832159de617a8188310abb324c30b8cb2a3408e, 4.3.dev1429+ga832159) resulted in errors like

Traceback (most recent call last):
  File "/orange/adamginsburg/ALMA_IMF/reduction/analysis/fullcube_quicklooks.py", line 154, in <module>
    cfrq = mcube.with_spectral_unit(u.GHz).spectral_axis.mean()
  File "/orange/adamginsburg/repos/spectral-cube/spectral_cube/spectral_cube.py", line 1366, in with_spectral_unit
    rest_value=rest_value)
  File "/orange/adamginsburg/repos/spectral-cube/spectral_cube/base_class.py", line 336, in _new_spectral_wcs
    velocity_convention=velocity_convention)
  File "/orange/adamginsburg/repos/spectral-cube/spectral_cube/spectral_axis.py", line 119, in determine_ctype_from_vconv
    in_physchar = PHYSICAL_TYPE_TO_CHAR[lin_cunit.physical_type]
KeyError: PhysicalType('frequency')

I think this PR will fix those.

codecov-commenter · 2021-05-01T18:44:29Z

Codecov Report

Merging #709 (0e5e9f9) into master (3d153ec) will increase coverage by 1.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #709      +/-   ##
==========================================
+ Coverage   76.32%   77.48%   +1.16%     
==========================================
  Files          24       24              
  Lines        5423     5557     +134     
==========================================
+ Hits         4139     4306     +167     
+ Misses       1284     1251      -33

Impacted Files	Coverage Δ
spectral_cube/spectral_axis.py	`93.65% <100.00%> (+0.06%)`	⬆️
spectral_cube/utils.py	`100.00% <0.00%> (ø)`
spectral_cube/stokes_spectral_cube.py	`92.00% <0.00%> (+0.45%)`	⬆️
spectral_cube/masks.py	`79.93% <0.00%> (+0.52%)`	⬆️
spectral_cube/dask_spectral_cube.py	`85.27% <0.00%> (+0.67%)`	⬆️
spectral_cube/cube_utils.py	`80.16% <0.00%> (+0.76%)`	⬆️
spectral_cube/conftest.py	`90.53% <0.00%> (+0.98%)`	⬆️
spectral_cube/base_class.py	`85.85% <0.00%> (+1.23%)`	⬆️
spectral_cube/spectral_cube.py	`77.65% <0.00%> (+1.76%)`	⬆️
spectral_cube/lower_dimensional_structures.py	`82.44% <0.00%> (+2.67%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d153ec...0e5e9f9. Read the comment docs.

astrofrog · 2021-05-02T07:45:38Z

Can you hold off on this as I think we should fix it in astropy

astrofrog · 2021-05-02T08:31:06Z

Actually feel free to go ahead, but I've noted in the relevant astropy PR that this broke rather than emit a deprecation warning

keflavich · 2021-05-06T13:35:37Z

@astrofrog can you review this, then? Should we go ahead and merge this?

I don't understand the failure.

e-koch · 2021-05-06T14:18:27Z

@keflavich -- More of the py38 errors we're seeing are from these string changes:

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
../../spectral_cube/spectral_axis.py F
097     Examples
098     --------
099     >>> determine_ctype_from_vconv('VELO-F2V', u.Hz)
100     'FREQ'
101     >>> determine_ctype_from_vconv('VELO-F2V', u.m)
102     'WAVE-F2W'
103     >>> determine_ctype_from_vconv('FREQ', u.m/u.s)  # doctest: +SKIP
104     ...
105     ValueError: A velocity convention must be specified
106     >>> determine_ctype_from_vconv('FREQ', u.m/u.s, velocity_convention=u.doppler_radio)
UNEXPECTED EXCEPTION: KeyError('speed/velocity')
Traceback (most recent call last):
  File "/Users/ekoch/anaconda3/lib/python3.8/doctest.py", line 1336, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest spectral_cube.spectral_axis.determine_ctype_from_vconv[3]>", line 1, in <module>
  File "/Users/ekoch/ownCloud/code_development/radio_astro_tools/spectral-cube/spectral_cube/spectral_axis.py", line 143, in determine_ctype_from_vconv
    out_physchar = PHYSICAL_TYPE_TO_CTYPE[str(unit.physical_type)]
KeyError: 'speed/velocity'

Here unit.physical_type = PhysicalType({'speed', 'velocity'}).

keflavich · 2021-05-06T14:29:22Z

Ahhh... I think we need changes here to accommodate speed/velocity, then.

e-koch · 2021-05-06T15:08:04Z

We might have a few other places where this breaks things. From a local run of the py38 dev test:

201 failed, 1157 passed, 155 skipped, 4 xfailed, 2 xpassed in 377.07s (0:06:17)

The errored tests are hiding a lot of actual failures.

e-koch · 2021-05-06T18:40:04Z

Most of the errors were from not passing the right unit conversions. Here's the upstream change for physical types: astropy/astropy#11691

Neither of the below issues are raised in the CI build.

It looks like we have some additional failures from upstream changes. In case they don't show up in the build with astropy dev:

One is an additional warning from a FITS header:

    @pytest.mark.skipif('not scipyOK')
    def test_parallel_bad_params(data_adv):
    
        # This function only makes sense for the plain SpectralCube class
    
        cube, data = cube_and_raw(data_adv, use_dask=False)
    
        with pytest.raises(ValueError,
                           match=("parallel execution was not requested, but "
                                  "multiple cores were: these are incompatible "
                                  "options.  Either specify num_cores=1 or "
                                  "parallel=True")):
            cube.spectral_smooth_median(3, num_cores=2, parallel=False,
                                        update_function=update_function)
    
        with warnings.catch_warnings(record=True) as wrn:
            cube.spectral_smooth_median(3, num_cores=1, parallel=True,
                                        update_function=update_function)
    
>       assert ("parallel=True was specified but num_cores=1. "
                "Joblib will be used to run the task with a "
                "single thread.") in str(wrn[-1].message)
E       assert 'parallel=True was specified but num_cores=1. Joblib will be used to run the task with a single thread.' in "'datfix' made the change 'Set MJD-OBS to 50982.687794 from DATE-OBS'."
E        +  where "'datfix' made the change 'Set MJD-OBS to 50982.687794 from DATE-OBS'." = str(FITSFixedWarning("'datfix' made the change 'Set MJD-OBS to 50982.687794 from DATE-OBS'."))
E        +    where FITSFixedWarning("'datfix' made the change 'Set MJD-OBS to 50982.687794 from DATE-OBS'.") = <warnings.WarningMessage object at 0x7ff059365c10>.message

spectral_cube/tests/test_spectral_cube.py:2330: AssertionError

The other is a convolution error that's from somewhere upstream (noted in convolution no longer compatible with scipy 1.5.0 (Windows) astropy/astropy#11651; but we're seeing this in non-Windows builds, too)

    def test_convolve_to_with_bad_beams(data_vda_beams, use_dask):
        cube, data = cube_and_raw(data_vda_beams, use_dask=use_dask)
    
>       convolved = cube.convolve_to(Beam(0.5*u.arcsec))

spectral_cube/tests/test_spectral_cube.py:2000: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
spectral_cube/utils.py:49: in wrapper
    return function(self, *args, **kwargs)
spectral_cube/spectral_cube.py:4037: in convolve_to
    newdata[ii, :, :] = convolve(img, kernel,
../../astropy/astropy/nddata/decorators.py:246: in wrapper
    result = func(data, *args, **kwargs)
../../astropy/astropy/convolution/convolve.py:731: in convolve_fft
    newshape = _next_fast_lengths(newshape)
../../astropy/astropy/convolution/convolve.py:85: in _next_fast_lengths
    return np.array([scipy.fft.next_fast_len(j, real=False) for j in shape])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <iterator object at 0x7ff0194f8ee0>

>   return np.array([scipy.fft.next_fast_len(j, real=False) for j in shape])
E   TypeError: good_size() takes no keyword arguments

keflavich · 2021-05-06T19:34:36Z

Yeah, astropy#11651 is a scipy issue, not a windows issue. It's an easy fix, but it's definitely upstream.

Not sure about the other one yet.

e-koch · 2021-05-06T20:51:03Z

We're not getting those errors I saw locally here, so I think it's ok.

Apart from the yt related failure, I think this is ready to review and merge

keflavich · 2021-05-06T20:53:50Z

spectral_cube/spectral_axis.py

@@ -72,6 +72,14 @@ def wcs_unit_scale(unit):
        if wu.is_equivalent(unit):
            return wu.to(unit)

+def parse_phys_type(unit):
+    '''
+    As of astropy 4.3.dev1499+g5b09f9dd9, the physical type of a speed if "speed".


is this supposed to say is "speed", not if "speed"?

and... isn't the problem that it was speed, and is now speed/velocity?

Oh. I did a replace all... will fix.

keflavich · 2021-05-06T20:55:01Z

OK, great, let's merge it - but, could you fix the docstring? Or at least clarify?

e-koch · 2021-05-06T21:21:15Z

@keflavich -- fixed. Good to merge from my end

recent versions of astropy require stringy versions of physical types

a1f8dba

keflavich force-pushed the stringify_unit_types branch from 01d6259 to a1f8dba Compare May 1, 2021 18:44

astrofrog mentioned this pull request May 2, 2021

Create a class to represent physical types astropy/astropy#11204

Merged

4 tasks

Fixes for speed physical types

0e5e9f9

keflavich commented May 6, 2021

View reviewed changes

Fix docstring

7076f73

keflavich merged commit bfc3350 into radio-astro-tools:master May 6, 2021

keflavich deleted the stringify_unit_types branch May 6, 2021 21:23

e-koch mentioned this pull request May 6, 2021

Handling larger range for BUNIT conversions #700

Merged

17 tasks

keflavich mentioned this pull request Sep 29, 2021

with_spectral_unit doesn't work. with astropy 4.3.1 #747

Closed

keflavich mentioned this pull request Jul 22, 2022

cube.with_spectral_unit gives KeyError: PhysicalType('frequency') #830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: stringy physical unit types #709

Bugfix: stringy physical unit types #709

keflavich commented May 1, 2021

codecov-commenter commented May 1, 2021 •

edited

Loading

astrofrog commented May 2, 2021

astrofrog commented May 2, 2021

keflavich commented May 6, 2021

e-koch commented May 6, 2021

keflavich commented May 6, 2021

e-koch commented May 6, 2021

e-koch commented May 6, 2021 •

edited

Loading

keflavich commented May 6, 2021

e-koch commented May 6, 2021

keflavich May 6, 2021

keflavich May 6, 2021

e-koch May 6, 2021

keflavich commented May 6, 2021

e-koch commented May 6, 2021

Bugfix: stringy physical unit types #709

Bugfix: stringy physical unit types #709

Conversation

keflavich commented May 1, 2021

codecov-commenter commented May 1, 2021 • edited Loading

Codecov Report

astrofrog commented May 2, 2021

astrofrog commented May 2, 2021

keflavich commented May 6, 2021

e-koch commented May 6, 2021

keflavich commented May 6, 2021

e-koch commented May 6, 2021

e-koch commented May 6, 2021 • edited Loading

keflavich commented May 6, 2021

e-koch commented May 6, 2021

keflavich May 6, 2021

Choose a reason for hiding this comment

keflavich May 6, 2021

Choose a reason for hiding this comment

e-koch May 6, 2021

Choose a reason for hiding this comment

keflavich commented May 6, 2021

e-koch commented May 6, 2021

codecov-commenter commented May 1, 2021 •

edited

Loading

e-koch commented May 6, 2021 •

edited

Loading