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 mask_safe handling in MapDataset.to_image #2498

Conversation

@luca-giunti
Copy link
Contributor

luca-giunti commented Oct 30, 2019

No description provided.

@luca-giunti luca-giunti requested a review from adonath Oct 30, 2019
@adonath adonath self-assigned this Oct 30, 2019
@adonath adonath added this to the 0.15 milestone Oct 30, 2019
Copy link
Member

adonath left a comment

Thanks a lot @luca-giunti. I've left two inline comments.

@@ -867,11 +868,20 @@ def to_image(self, spectrum=None, keepdims=True):
dataset : `MapDataset`
Map dataset containing images.
"""
self.counts.data[~self.mask_safe.data] = 0

This comment has been minimized.

Copy link
@adonath

adonath Oct 30, 2019

Member

Note that this modifies the data in place and leaves the parent MapDataset in a modified state. This we should avoid and make a copy of the data instead. This can be achieved e.g. by just multiplying the mask as well:

from gammapy.maps import WcsNDMap
import numpy as np

m = WcsNDMap.create(width=(8, 5))
m.data += 1

mask = np.ones(m.geom.data_shape)
mask[10:20] = 0

m_masked = m * mask

m_masked.plot()

We could also add a weights argument to .sum_over_axis().

counts = self.counts.sum_over_axes(keepdims=keepdims)
exposure = _map_spectrum_weight(self.exposure, spectrum)
exposure = exposure.sum_over_axes(keepdims=keepdims)
background = self.background_model.evaluate().sum_over_axes(keepdims=keepdims)

slices_num = self.mask_safe.geom.get_axis_by_name("ENERGY").nbin

This comment has been minimized.

Copy link
@adonath

adonath Oct 30, 2019

Member

Note that the + operator corresponds to the logical or operation:

import numpy as np
mask_1 = np.array([[1, 0]], dtype=bool)
mask_2 = np.array([[1], [0]], dtype=bool)

print(mask_1 + mask_2)
print()
print(np.logical_or(mask_1, mask_2))

So you can basically use .sum_over_axis() for the mask as well (as long as the dtype is bool)

@cdeil cdeil changed the title Implemented mask_safe handling in MapDataset.to_image() Add mask_safe handling in MapDataset.to_image Oct 30, 2019
@cdeil cdeil added this to In progress in gammapy.cube via automation Oct 30, 2019
mask_safe_image = self.mask_safe.slice_by_idx({"energy": 0})
for idx in range(1, slices_num):
mask_slice = self.mask_safe.slice_by_idx({"energy": idx})
mask_safe_image = mask_safe_image or mask_slice

This comment has been minimized.

Copy link
@cdeil

cdeil Oct 31, 2019

Member

The or operator doesn't work for masks.

You should use | or & operators always:

In [6]: np.array([0, 0, 1, 1], dtype=bool) & np.array([1, 0, 1, 0], dtype=bool)                                                                                                                   
Out[6]: array([False, False,  True, False])

In [7]: np.array([0, 0, 1, 1], dtype=bool) or np.array([1, 0, 1, 0], dtype=bool)                                                                                                                  
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-7-8bce39258b03> in <module>
----> 1 np.array([0, 0, 1, 1], dtype=bool) or np.array([1, 0, 1, 0], dtype=bool)

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Don't use +, it's not appropriate. Always use & (logical and) or | (logical or)`.

This comment has been minimized.

Copy link
@adonath

adonath Oct 31, 2019

Member

I agree, I think the simplest and clearest expression to reduce the 3D mask would be:

idx = mask_safe.geom.get_axis_index_by_name("energy")
mask_image = np.logical_or(mask_safe, axis=idx)
@adonath adonath merged commit 8ec4399 into gammapy:master Nov 4, 2019
8 of 9 checks passed
8 of 9 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: 5 new issues, 54 updated code elements – Tests: passed
Details
gammapy.gammapy Build #20191101.1 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
gammapy.cube automation moved this from In progress to Done Nov 4, 2019
@adonath

This comment has been minimized.

Copy link
Member

adonath commented Nov 4, 2019

Thanks @luca-giunti!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
gammapy.cube
  
Done
3 participants
You can’t perform that action at this time.