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

Implement WcsNDMap.stack() method #2446

Merged
merged 8 commits into from Oct 14, 2019
Merged

Conversation

@adonath
Copy link
Member

adonath commented Oct 8, 2019

This PR implements a WcsNDMap.stack() method. The .stack() methode is meant to be used together with WcsNDMap.cutout() to handle the common use-case of processing a cutout of a map and the pasting the result into a larger map. To achieve this in a consistent manner a WcsGeom.cutout_info is added, which contains a reference to the parent geom as well as the parent and cutout slices. Later the info could be extended e.g. to also contain views e.g. into larger coordinate arrays.

@adonath adonath self-assigned this Oct 8, 2019
@adonath adonath added this to the 0.15 milestone Oct 8, 2019
gammapy/maps/wcsnd.py Outdated Show resolved Hide resolved
Other map to stack
weights : `~numpy.ndarray`
Other map to be used as weights.
check : bool

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 8, 2019

Contributor

Is the check argument really needed? Why not always check if the géométrie is really a cutout from the other?

This comment has been minimized.

Copy link
@adonath

adonath Oct 11, 2019

Author Member

Currently there is the problem, that we don't create necessarily all the cutouts from the same parent geometry. The situation will improve a bit after #2450. Once #2450 is merged I'll rebase this PR and re-address your comment.

This comment has been minimized.

Copy link
@adonath

adonath Oct 14, 2019

Author Member

I removed the check argument again...

return self._init_copy(geom=geom, data=data)
return self._init_copy(geom=geom_cutout, data=data)

def stack(self, other, weights=None, check=True):

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 8, 2019

Contributor

Since this is doing the same thing as coadd except that it works for a cutout why not name this coadd_cutout? stack is already used for datasets. This might be confusing since this method could be used for other usages than stacking, no?

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 8, 2019

Contributor

Another option is to add a test in coadd so that if self.is_cutout_from(other) the simple slice approach is used. Then you don't need to check in each stack method what to use.

This comment has been minimized.

Copy link
@adonath

adonath Oct 14, 2019

Author Member

I agree there is an overlap in functionality of .stack() and .coadd(). So .coadd() is the completely general, but slow method and .stack() is the method to be used in the single use-case in combination with .cutout(). I have a minimal preference to not merge both methods into one for two minor reasons:

  1. I like the mirrored API between MapDataset and Map, so MapDataset.stack(), calls into Map.stack() and PSFMap.stack() etc.
  2. Adding the fast track to .coadd() complicates the code in .coadd() to a lot of cases, with checking for aligned geometries, checking if .cutout_info is set etc. I think it's easier to maintain the .cutout() and .stack() use-case in a separate place.
@@ -360,23 +357,24 @@ def stack(self, other):
"""
if self.counts and other.counts:
self.counts.data[~self.mask_safe] = 0
self.counts.coadd(other.counts, weights=other.mask_safe)
self.counts.stack(other.counts, weights=other.mask_safe, check=False)

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 8, 2019

Contributor

Why check = False? Could there be cases where someone tries to stack datasets which are not cutouts?

This comment has been minimized.

Copy link
@adonath

adonath Oct 14, 2019

Author Member

I removed the check option...

"""

_slice_spatial_axes = slice(0, 2)
_slice_non_spatial_axes = slice(2, None)
is_hpx = False

def __init__(self, wcs, npix, cdelt=None, crpix=None, axes=None):
def __init__(self, wcs, npix, cdelt=None, crpix=None, axes=None, cutout_info=None):

This comment has been minimized.

Copy link
@registerrier

registerrier Oct 8, 2019

Contributor

Should cutout_info be an argument in the constructor? It could be added dynamically after the cutout is performed

@adonath adonath force-pushed the adonath:add_map_stack_method branch from d4263a1 to b02c079 Oct 14, 2019
@adonath adonath merged commit 8e6fe77 into gammapy:master Oct 14, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 4 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191014.17 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
@adonath adonath changed the title Implement `WcsNDMap.stack()` method Implement WcsNDMap.stack() method Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.