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

Add paste, cutout and look_up methods to SkyMap class #559

Merged
merged 10 commits into from Jun 10, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jun 8, 2016

This PR adds cutout(), paste() and lookup_max() methods to the SkyMap class and removes it from gammapy.image.utils

@cdeil cdeil added this to the 0.5 milestone Jun 9, 2016

@cdeil cdeil self-assigned this Jun 9, 2016

@adonath adonath force-pushed the adonath:paste_method_for_skymap_class branch from fc8af94 to 1afa161 Jun 9, 2016

@@ -63,3 +63,72 @@ The sky map can be easily displayed with an image viewer, by calling ``skymap.sh
counts = SkyMap.read(filename)
counts.name = 'Counts'
counts.show()
Working with the cutout and paste method

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

I have a few docs suggestions.

Make the title shorter "Cutout and paste" so that it's nicer in the navigation side bar.

Add a label in front of high-level docs sections, in this case e.g.

.. _skymap-cutpaste:

Cutout and paste
----------------

And then in the docstring of cutout and paste, link to this high-level docs section via

See :ref:`skymap-cutpaste` for information how to cut and paste sky images.

and then don't add another example to those docstrings.

Working with the cutout and paste method
----------------------------------------
The `~gammapy.image.SkyMap` class offers `.paste()` and `.cutout()`

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Maybe mark up the paste and cutout method so that in the HTML version it's links directly to the methods?

This comment has been minimized.

@adonath

adonath Jun 10, 2016

Member

I didn't want to use mark up ticks in the header, but in the first sentence .paste() and .cutout() are linked.

The `~gammapy.image.SkyMap` class offers `.paste()` and `.cutout()`
methods, that can be used to cut out smaller parts of a sky map.
Here we cut out a 2 deg x 2 deg patch out of an example image:

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Is it 2 x 2 deg or 1 x 1 deg?
Below you pass size = Quantity([1, 1], 'deg') ... so it's 1 x 1 deg, no?

This comment has been minimized.

@adonath

adonath Jun 10, 2016

Member

Fixed

Parameters
----------
skymap: `SkyMap`

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Missing space before colon:

skymap : `SkyMap`
elif method == 'replace':
self.data[ylo:yhi, xlo:xhi] = skymap.data[ylo_c:yhi_c, xlo_c:xhi_c]
else:
raise ValueError('Invalid method: {0}'.format(method))

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Change {0} to {} (for consistency with all other string format calls in Gammapy).
(The position index is only needed for Python 2.6, which we don't support any more)

This comment has been minimized.

@adonath

adonath Jun 10, 2016

Member

Fixed

Tuple of `~astropy.units.Angle`, specifying the size of
the cutout.
Examples

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

I'd suggest to remove this Examples section and instead in the long description (above the Parameter) link to the high-level docs example. In Astropy they do this cross-linking of low-level and API docs more and more (latest example, and I think it's really convenient for users.

This comment has been minimized.

@adonath
----------
position : `~astropy.coordinates.SkyCoord`
Position of the center of the sky map to cut out.
size : tuple

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Shouldn't we allow a single angle for convenience, i.e. a cutout of 1 deg is equivalent to a 1 deg x 1 deg tuple?

This comment has been minimized.

@adonath

adonath Jun 10, 2016

Member

That was actually already possible, but wasn't documented. I just copied over the documentation from Cutout2D...

from astropy.coordinates import Latitude, Longitude, Angle
from astropy.utils import lazyproperty
from ..utils.scripts import make_path

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

remove unused make_path import in this file.

This comment has been minimized.

@adonath
assert_allclose((359.93, -0.01), (pos.galactic.l.deg, pos.galactic.b.deg))
def test_lookup_max_region(self):
from ...extern.regions.shapes import CircleSkyRegion

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Move import to the top of the file.

This comment has been minimized.

@adonath
BINSZ = 0.02
# setup coordinate images
lon = SkyMap.empty(nxpix=201, nypix=201, binsz=BINSZ)

This comment has been minimized.

@cdeil

cdeil Jun 9, 2016

Member

Does it help to have 40401 pixels?
Maybe use 11 x 10 pixels or fewer?

This is faster, but more importantly ... if tests fail, very small arrays means it's easier to print the on the console and quickly see what's going on.

This comment has been minimized.

@adonath
@cdeil

This comment has been minimized.

Member

cdeil commented Jun 9, 2016

I've left some inline comments ... all minor stuff I think.
I didn't think through the critical code like the int rounding.
For me it's OK if you just go ahead and merge this, or, if you want me to review this before we merge, let me know.

@adonath adonath force-pushed the adonath:paste_method_for_skymap_class branch from 464e828 to a321ec5 Jun 10, 2016

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 10, 2016

Looks great!
Merging this now.

@cdeil cdeil merged commit 1b0671f into gammapy:master Jun 10, 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:paste_method_for_skymap_class branch Nov 20, 2018

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