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

Improve SkyCube class #753

Merged
merged 13 commits into from Nov 4, 2016

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Nov 3, 2016

This PR refactors the SkyCube class as following:

  • Removed .coordinates() and .solid_angle() methods. These can be now accessed via SkyCube.spatial.coordinates(), where SkyCube.spatial is a property returning a reference SkyImage
  • Add a simple SkyCube.show() method that works in notebooks with a slider
  • Simplify SkyCube.reproject() by reusing SkyImage.reproject()
  • Replace SkyCube.world2pix() by SkyCube. wcs_skycoord_to_pixel() which takes SkyCoord objects and makes it consistent with SkyImage
  • Replace SkyCube.pix2world() by SkyCube. wcs_pixel_to_skycoord() which returns SkyCoord objects and makes it consistent with SkyImage
  • Rename SkyCube.flux() to SkyCube.lookup(), which is now consistent with SkyImage

@adonath adonath added this to the 0.5 milestone Nov 3, 2016

@adonath adonath self-assigned this Nov 3, 2016

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 3, 2016

Member

@adonath - Thanks!

The one change I don't like is ref_sky_image -> spatial.
With "spatial" I have no idea from the name what it is.
It's a sky image, so having "sky_image" in the name results in more intuitive / readable code, no?

Member

cdeil commented Nov 3, 2016

@adonath - Thanks!

The one change I don't like is ref_sky_image -> spatial.
With "spatial" I have no idea from the name what it is.
It's a sky image, so having "sky_image" in the name results in more intuitive / readable code, no?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 3, 2016

Member

I like the name SkyCube.spatial in the context, that it is typically used. I.e. I found it more intuitive to read SkyCube.spatial.coordinates() or SkyCube.spatial.footprint() instead of SkyCube.ref_sky_image.coordinates() or SkyCube.ref_sky_image.footprint() . I see it more like a bonus for better readable code, because you can always use SkyCube.sky_image(0).coordinates().

Member

adonath commented Nov 3, 2016

I like the name SkyCube.spatial in the context, that it is typically used. I.e. I found it more intuitive to read SkyCube.spatial.coordinates() or SkyCube.spatial.footprint() instead of SkyCube.ref_sky_image.coordinates() or SkyCube.ref_sky_image.footprint() . I see it more like a bonus for better readable code, because you can always use SkyCube.sky_image(0).coordinates().

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 3, 2016

Member

I found it more intuitive to read SkyCube.spatial.coordinates() or SkyCube.spatial.footprint() instead of SkyCube.ref_sky_image.coordinates() or SkyCube.ref_sky_image.footprint() .

I think that's because you just wrote it and know what king of object spatial is.
IMO it's clear that the more descriptive name "ref_sky_image" is more intuitive ... makes it clear that it's a SkyImage object.

you can always use SkyCube.sky_image(0).coordinates().

Yes. And I wasn't sure if I should introduce the ref_sky_image property.
I'm +1 to keep it because I thought it is good to have this pattern as a separate property, not just documented as part of the cube.sky_image docstring.

But that's a different question / suggestion to the change proposed here, which is to rename ref_sky_image to spatial.

The other change here is property -> lazyproperty, which I think is a good one.

Member

cdeil commented Nov 3, 2016

I found it more intuitive to read SkyCube.spatial.coordinates() or SkyCube.spatial.footprint() instead of SkyCube.ref_sky_image.coordinates() or SkyCube.ref_sky_image.footprint() .

I think that's because you just wrote it and know what king of object spatial is.
IMO it's clear that the more descriptive name "ref_sky_image" is more intuitive ... makes it clear that it's a SkyImage object.

you can always use SkyCube.sky_image(0).coordinates().

Yes. And I wasn't sure if I should introduce the ref_sky_image property.
I'm +1 to keep it because I thought it is good to have this pattern as a separate property, not just documented as part of the cube.sky_image docstring.

But that's a different question / suggestion to the change proposed here, which is to rename ref_sky_image to spatial.

The other change here is property -> lazyproperty, which I think is a good one.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 3, 2016

Member

@joleroi Any opinion on SkyCube.spatial vs. SkyCube.ref_sky_image?

Member

adonath commented Nov 3, 2016

@joleroi Any opinion on SkyCube.spatial vs. SkyCube.ref_sky_image?

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Nov 4, 2016

Contributor

Why not call it ref_sky_image but return an empty sky image with the right geometry. Than the property would be somehow useful, otherwise you just avoid typing

cube.sky_image[0].coordinates

which is actually shorter than

cube.ref_sky_image.coordinates
Contributor

joleroi commented Nov 4, 2016

Why not call it ref_sky_image but return an empty sky image with the right geometry. Than the property would be somehow useful, otherwise you just avoid typing

cube.sky_image[0].coordinates

which is actually shorter than

cube.ref_sky_image.coordinates
@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 4, 2016

Member

@joleroi Thanks! That's how it used to be. I'll revert my change.

Member

adonath commented Nov 4, 2016

@joleroi Thanks! That's how it used to be. I'll revert my change.

@adonath adonath merged commit 15fe962 into gammapy:master Nov 4, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adonath adonath deleted the adonath:rework_skycube_class branch Nov 4, 2016

@adonath adonath referenced this pull request Nov 14, 2016

Closed

Finish SkyCube implementation #121

6 of 8 tasks complete

@cdeil cdeil changed the title from Rework SkyCube class to Improve SkyCube class Nov 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment