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 MapDataset.stack #2381

Merged
merged 3 commits into from Sep 21, 2019

Conversation

@AtreyeeS
Copy link
Member

commented Sep 19, 2019

This is a preliminary implementation for MapDataset.stack. It does not check if other is a cutout of self, but implicitly assumes so.

if self.gti:
self.gti = self.gti.stack(other.gti).union()

# How to stack the mask?

This comment has been minimized.

Copy link
@AtreyeeS

AtreyeeS Sep 19, 2019

Author Member

We should typically want to do logical or on the mask as in SpectrumDataset.stack().
However, mask_fit is not a Map object but a numpy array. How do we take care of the fact that other.mask_fit is a cutout of self.mask_fit and will not have the same dimensions?

This comment has been minimized.

Copy link
@adonath

adonath Sep 20, 2019

Member

This is a good point and probably comes in as an argument to make the mask_fit and mask_safe a Map object as well. For now I think it's OK to temporarily create a map object and use it to stack the mask like so:

mask_safe = Map.from_geom(self.counts.geom, data=self.mask_safe)
mask_safe_other = Map.from_geom(other.counts.geom, data=other.mask_safe)

Note that the + operator on bool types should already give what we want:

import numpy as np
a = np.array([1, 1, 0, 0], dtype=bool)
b = np.array([1, 0, 1, 0], dtype=bool)
a + b

Gives:

array([ True,  True,  True, False])
Copy link
Member

left a comment

Thanks a lot @AtreyeeS! I've left a few comments, otherwise it looks already very good to me.

other: `~gammapy.cube.MapDataset`
the MapDatasets to be stacked with this one.
Returns

This comment has been minimized.

Copy link
@adonath

adonath Sep 20, 2019

Member

As the stacking is done in place, nothing is returned. So just remove this section...

if self.gti:
self.gti = self.gti.stack(other.gti).union()

# How to stack the mask?

This comment has been minimized.

Copy link
@adonath

adonath Sep 20, 2019

Member

This is a good point and probably comes in as an argument to make the mask_fit and mask_safe a Map object as well. For now I think it's OK to temporarily create a map object and use it to stack the mask like so:

mask_safe = Map.from_geom(self.counts.geom, data=self.mask_safe)
mask_safe_other = Map.from_geom(other.counts.geom, data=other.mask_safe)

Note that the + operator on bool types should already give what we want:

import numpy as np
a = np.array([1, 1, 0, 0], dtype=bool)
b = np.array([1, 0, 1, 0], dtype=bool)
a + b

Gives:

array([ True,  True,  True, False])

if self.counts:
if other.mask_fit:
other.counts = other.counts * other.mask_fit

This comment has been minimized.

Copy link
@adonath

adonath Sep 20, 2019

Member

I think we should not modify the other object when stacking. For the "in place" stacking we should only modify self. My proposal would be to add a weights argument (just as a numpy array) to Map.coadd(), which is multiplied to the values stacked in the map. So the code here would be:

self.counts.coadd(other.counts, weights=other.mask_safe)

Also note that we want to use mask_safe for the stacking not mask_fit.


if other.mask_fit:
other.background_model.map = other.background_model.map * other.mask_fit
self.background_model.map.coadd(other.background_model.evaluate())

This comment has been minimized.

Copy link
@adonath

adonath Sep 20, 2019

Member

Same comment as above, maybe we should add a weights argument to Map.coadd().

@cdeil cdeil added the cleanup label Sep 20, 2019
@cdeil cdeil added this to the 0.14 milestone Sep 20, 2019
@cdeil cdeil added this to To do in Map analysis via automation Sep 20, 2019
@AtreyeeS

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2019

Thanks @adonath ! Addressed the changes. For now, I am going with coadd, and we can shift to a faster implementation (add a Map.stack() or improve Map.coadd() for aligned geoms as noted in the TODO

@adonath

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Thanks a lot @AtreyeeS!

@adonath adonath merged commit 6e4cf8f into gammapy:master Sep 21, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 5 new issues, 2 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20190921.3 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
Map analysis automation moved this from To do to Done Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Map analysis
  
Done
3 participants
You can’t perform that action at this time.