radio-astro-tools / spectral-cube

Library for reading and analyzing astrophysical spectral data cubes
http://spectral-cube.rtfd.org
BSD 3-Clause "New" or "Revised" License
95 stars 62 forks source link

Bugfix: change isinstance checks to base class #782

Closed keflavich closed 2 years ago

keflavich commented 2 years ago

Right now, cube*cube will trigger _apply_everywhere instead of _cube_on_cube_operation if cube is a DaskSpectralCube. This should fix that mistake.

codecov-commenter commented 2 years ago

Codecov Report

Merging #782 (be7de8a) into master (9b15939) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #782   +/-   ##
=======================================
  Coverage   77.91%   77.91%           
=======================================
  Files          24       24           
  Lines        5818     5818           
=======================================
  Hits         4533     4533           
  Misses       1285     1285           
Impacted Files Coverage Δ
spectral_cube/spectral_cube.py 76.68% <100.00%> (ø)

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 9b15939...be7de8a. Read the comment docs.

keflavich commented 2 years ago

Are you guys OK with adding mock as a dependency? It seems super useful for testing things like this, so I'm happy to add it, but I'm mostly just using examples c&p'd from SO

keflavich commented 2 years ago

Why are we using BaseSpectralCube in the CASA reader?

io_registry.register_reader('casa', BaseSpectralCube, load_casa_image)
io_registry.register_reader('casa_image', BaseSpectralCube, load_casa_image)
io_registry.register_identifier('casa', BaseSpectralCube, is_casa_image)
dhomeier commented 2 years ago

Why are we using BaseSpectralCube in the CASA reader?

No idea ;-) But it seems the same problem would have arisen with any VaryingResolutionSpectralCube instance.

keflavich commented 2 years ago

Ah, you're right, and that's probably why. OK, good catch. I think this regression now covers everything, and it was a mistake to insist on things being subclasses of SpectralCube, since VaryingResolutionSpectralCube is not a subclass of SpectralCube

keflavich commented 2 years ago

oh ffs... openfiles again