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

Attach custom beam #433

Merged
merged 16 commits into from
Oct 6, 2017
Merged

Conversation

e-koch
Copy link
Contributor

@e-koch e-koch commented Oct 3, 2017

From #432. A beam object can now be attached with:

newcube = cube.with_beam(beam)
newproj = projection.with_beam(beam)

Since radio-beam is now a dependency, I've removed the read_beam option from SpectralCube. We need to keep it for Projection though since there remains the issue with non-matching celestial axes for some slices.

@e-koch e-koch requested a review from keflavich October 3, 2017 17:15
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 74.941% when pulling e5ff430 on e-koch:attach_custom_beam into 668e322 on radio-astro-tools:master.

@keflavich
Copy link
Contributor

I would prefer this be a with_beam method; nearly everything else we do related to modifying cubes forces the creation of a new cube object, and it feels weird having a one-off attach method. If a beam is really needed as part of the original cube, it should be established during instantiation.

if not isinstance(beam, Beam):
raise TypeError("beam must be a radio_beam.Beam object.")

if beam is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

we're already in the else of if beam is None, so this if could just be deleted with no change of indentation, I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I'll have to duplicate the three lines below that if without it for when the beam is loaded from the header. The other option would be to move loading a beam from the header somewhere into io.fits

Copy link
Contributor

Choose a reason for hiding this comment

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

right, good point, OK.

@e-koch
Copy link
Contributor Author

e-koch commented Oct 5, 2017

Agreed. I changed both to be with_beam and return a new object.

It highlighted one issue in 2D objects when the slice is taken along a position-velocity slice:

___________________________ TestSpectralCube.test_getitem[vda_beams-trans8] ____________________________

self = <spectral_cube.tests.test_spectral_cube.TestSpectralCube object at 0x7f33e9101190>
name = 'vda_beams', trans = [0, 2, 1]

    @pytest.mark.parametrize(('name', 'trans'), translist)
    def test_getitem(self, name, trans):
        c, d = cube_and_raw(name + '.fits')
    
        expected = np.squeeze(d.transpose(trans))
    
        assert_allclose(c[0,:,:].value, expected[0,:,:])
>       assert_allclose(c[:,:,0].value, expected[:,:,0])

spectral_cube/tests/test_spectral_cube.py:257: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
spectral_cube/spectral_cube.py:2862: in __getitem__
    meta=meta)
spectral_cube/lower_dimensional_structures.py:236: in __new__
    self.beam = beam
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <Slice [[ 0.54671028, 0.96958463, 0.93949894],
        [ 0.59789998, 0.0884925...2873751, 0.28093451],
        [ 0.14092422, 0.07455064, 0.77224477]] Jy / beam>
obj = [Beam: BMAJ=0.10000000149 arcsec BMIN=0.40000000596 arcsec BPA=0.0 deg, Beam: BMAJ=0.20000000298 arcsec BMIN=0.3000000... arcsec BMIN=0.20000000298 arcsec BPA=60.0 deg, Beam: BMAJ=0.40000000596 arcsec BMIN=0.10000000149 arcsec BPA=30.0 deg]

    @beam.setter
    def beam(self, obj):
    
        if not isinstance(obj, Beam):
>           raise TypeError("beam must be a radio_beam.Beam object.")
E           TypeError: beam must be a radio_beam.Beam object.

We should probably just not allow this type of slicing with a VRSC.

@e-koch
Copy link
Contributor Author

e-koch commented Oct 5, 2017

@keflavich - separate PR for the above error? One test should fail on travis.

@keflavich
Copy link
Contributor

It's OK to fix it here if you want

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 74.985% when pulling a697f51 on e-koch:attach_custom_beam into 668e322 on radio-astro-tools:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 74.964% when pulling ee56618 on e-koch:attach_custom_beam into 668e322 on radio-astro-tools:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 74.964% when pulling cca53b8 on e-koch:attach_custom_beam into 668e322 on radio-astro-tools:master.

@e-koch e-koch merged commit 93fa50f into radio-astro-tools:master Oct 6, 2017
@e-koch e-koch deleted the attach_custom_beam branch October 6, 2017 19:33
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.

3 participants