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

Add up- and downsampling to SkyImage #653

Merged
merged 10 commits into from Jul 29, 2016

Conversation

@adonath
Copy link
Member

@adonath adonath commented Jul 28, 2016

This PR adds the up- and downsampling to SkyImage and makes the following related changes:

  • Add SkyImage.upsample() and SkyImage.downsample() methods and tests
  • Modify SkyImage.pad() to take an additional shape parameter and recompute the wcs specs.
  • Add a SkyImage.crop() method to crop a skyimage and recompute the wcs specs.
  • Make compute_ts_map and compute_ts_map_multiscale work with SkyImage and SkyImageCollection
@cdeil cdeil added this to the 0.5 milestone Jul 28, 2016
@cdeil cdeil self-assigned this Jul 28, 2016
Parameters
----------
factor : int
Down sampling factor, must be power of two.

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

Down sampling factor, must be power of two.

I think a downsampling factor of 3 is OK, no?
Adapt description?

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

from skimage.measure import block_reduce

shape = self.data.shape
factor = int(factor)

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

The doc says factor has to be int.
I'm -1 an auto-truncating floats by calling factor = int(factor) here.
Either remove this line, expecting the user to follow the docs.
Or if you want to ensure int, instead of silently truncating, I'd suggest throwing an error if the input isn't an int.

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

Parameters
----------
factor : int
up sampling factor, must be power of two.

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

Again: upsampling factor doesn't have to be factor of two, does it?

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

wcs = _get_resampled_wcs(self, factor, downsampled=True)
return SkyImage(data=data, wcs=wcs)

def upsample(self, factor, order=3):

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

Looks like order=3 is also the default for scipy.ndimage.zoom:
http://docs.scipy.org/doc/scipy/reference/generated/scipy.ndimage.zoom.html

How about changing this to

def upsample(self, factor, **kwargs)

and passing kwargs along to scipy.ndimage.zoom?

Then the caller has more power.

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

@@ -898,3 +998,17 @@ def __str__(self):
info += self[name].__str__()
info += '\n'
return info


def _get_resampled_wcs(skyimage, factor, downsampled):

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

Only pass wcs in here, not skyimage.

I think fermipy has a slightly more flexible wcs utils function for this ... I'll start comparing and merging the two tomorrow.

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

@@ -261,6 +261,56 @@ def test_cutout_paste_wcs_error(self):
with pytest.raises(WcsError):
image.paste(cutout)

@pytest.mark.parametrize(('shape', 'factor', 'proj'), [((4, 4), 2, 'CAR'),

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

I'd suggest to always use asymmetric shapes for tests.
Then flipped axis index errors are noticed.

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

Done

# for https://github.com/numpy/numpy/issues/7112
mode = str(mode)
if shape is not None:
xdiff = shape[1] - self.data.shape[1]

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

@adonath - Could you please use the constants self.ax_idx.x and self.ax_idx.y here instead of 1 and 0 directly?

I know you know when to use FITS / WCS and when to use Numpy axis order and that they are flipped, but not everyone does and I find the code more readable if that's made explicit via constants.

Also -- Do you think this should be public or private?

If public, could you add a one-line docstring below?
https://gammapy.readthedocs.io/en/latest/api/gammapy.image.SkyImage.html#gammapy.image.SkyImage.ax_idx

If private, could you add an underscore so that this doesn't show up in the API docs?

This comment has been minimized.

@adonath

adonath Jul 28, 2016
Author Member

This line is obsolete, but I will use it in other places.

I think right now I would make it private, but later one could think about adding some further info to the Axis object (e.g. SkyImage.axes.x.unit, SkyImage.axes.x.idx or SkyCube.axes.energy.unit) and make it public.

@adonath adonath force-pushed the adonath:skyimage_up_and_downsampling branch from ac09e4d to bf76b97 Jul 28, 2016
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 28, 2016

I had another quick look. All looks good to me now. Thanks!

@adonath
Copy link
Member Author

@adonath adonath commented Jul 28, 2016

Thanks @cdeil! I've changed SkyImage.pad() and SkyImage.crop() as we discussed offline. I moved the shape computing utility functions to gammapy.utils.array, to keep the namespace in gammapy.image cleaner.

@@ -932,3 +1014,17 @@ def __str__(self):
info += self[name].__str__()
info += '\n'
return info


def _get_resampled_wcs(wcs, factor, downsampled):

This comment has been minimized.

@cdeil

cdeil Jul 28, 2016
Member

FYI: The wcs resampling helper function could go here:
https://github.com/gammapy/gammapy/blob/master/gammapy/utils/wcs.py

But I don't care if you do this now, I'll look at
https://github.com/fermiPy/fermipy/blob/master/fermipy/wcs_utils.py
and our utils soon and then start making PRs to move things into the right places / make them simple and consistent / add tests.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 28, 2016

Missing @requires_dependency('scipy') and @requires_dependency('skimage') for the up and downsample tests: https://travis-ci.org/gammapy/gammapy/jobs/148055171#L1507

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 29, 2016

@adonath - Thanks!

pytest skips should be OK. I'm merging this now.

@cdeil cdeil merged commit 2532a37 into gammapy:master Jul 29, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath adonath deleted the adonath:skyimage_up_and_downsampling branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants