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

Several serious bug fixes #506

Merged
merged 33 commits into from
Nov 30, 2018
Merged

Conversation

keflavich
Copy link
Contributor

  1. In downsampling, there was an incorrectly named variable, so the mask was computed incorrectly. We clearly don't have a test for this yet.
  2. The convolve_to [x] and reproject [ ] functions were loading the whole cube into memory prior to running, which is bad.
  3. both convolve_to and reproject are creating in-memory full-sized cubes to store the output of the process instead of using the memmap technology of apply_function_spectral_parallel

Copy link
Contributor

@e-koch e-koch left a comment

Choose a reason for hiding this comment

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

LGTM

@keflavich
Copy link
Contributor Author

@e-koch this one needs a re-review. Things kept breaking that required other things to be fixed... I'm not sure I'm done diving down this rabbit hole

the internet that claim this was a python-core issue that was (maybe?)
fixed.  However, it is actually impossible for this to be a python issue
alone, it has to be something in py.test also, since we didn't encounter
it at all for years.

pytest-dev/pytest#1217
@e-koch
Copy link
Contributor

e-koch commented Nov 30, 2018

@keflavich, there's still something funky going on with masking in the 2D objects:

>>> cube, data = cube_and_raw('adv.fits')
>>> masked_cube = cube.with_mask(np.zeros_like(cube, bool))
>>> masked_cube[0]
<Slice [[nan, nan],
        [nan, nan],
        [nan, nan]] K>
>>> masked_cube[0].mask.include()
array([[ True,  True],
       [ True,  True],
       [ True,  True]])
# Should all be false
>>> masked_cube.mask
<spectral_cube.masks.CompositeMask object at 0x7fed067e3e48>
>>> masked_cube.mask.include()[0]
array([[False, False],
       [False, False],
       [False, False]])
# Mask from the cube is right
>>> masked_cube[:, 0, 0]
<OneDSpectrum [nan,nan,nan,nan] K>
>>> masked_cube[:, 0, 0].mask.include()
array([False, False, False, False])
# Mask for 1D objects is right

We'll need tests for the 2D masking, too.

@e-koch
Copy link
Contributor

e-koch commented Nov 30, 2018

Here's the culprit for why the WCS checks are failing for the 2D masks: https://github.com/radio-astro-tools/spectral-cube/blob/master/spectral_cube/wcs_utils.py#L214. The spatial axes get reversed. But sorting them in the right order also causes problems.

The data WCS gets sliced differently: https://github.com/radio-astro-tools/spectral-cube/blob/master/spectral_cube/spectral_cube.py#L1163. So we need some fix for the former or to include the latter in a more general function

@e-koch
Copy link
Contributor

e-koch commented Nov 30, 2018

Correction: Sorting the axes in the right order does work. The tests that still fail are for slices that leave only 1 celestial axis. Now I remember why we had problems with 2D masking in the past...

@keflavich
Copy link
Contributor Author

Indeed, #395 was our attempt to fix this. I think we'll need to impose a severe hack to get this to work in the meantime. Possibly, we need to just remove the CTYPE keyword entirely for unmatched celestial axis cases. I'll experiment with that.

@keflavich
Copy link
Contributor Author

@e-koch what do you think of merging this with coverage tests sadly disabled?

@e-koch
Copy link
Contributor

e-koch commented Nov 30, 2018

Fine with me. Hopefully, we can add them back in short order.

@keflavich keflavich merged commit f038a40 into radio-astro-tools:master Nov 30, 2018
@keflavich keflavich deleted the several_fixes branch November 30, 2018 23:25
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