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 filter option to MapFit #1702

Merged
merged 4 commits into from Aug 15, 2018

Conversation

2 participants
@adonath
Member

adonath commented Aug 14, 2018

This PR implements the handling of an exclusion mask in the MapFit object. I tried a few possible implementations, such as applying the exclusion mask to the stat array and in the total_stat() method.
In the end the most efficient solution was to apply the mask directly to the exposure map and setting the exposure of the corresponding pixels to zero. The downside is, that the exposure map is now different for MapFit and MapEvaluater. There the question whether MapEvaluater should take an exclasion mask as well. I was sure whether this makes sense, but maybe it does?

@adonath adonath added the feature label Aug 14, 2018

@adonath adonath added this to the 0.8 milestone Aug 14, 2018

@adonath adonath self-assigned this Aug 14, 2018

@adonath adonath added this to To do in Map analysis via automation Aug 14, 2018

@adonath adonath requested review from cdeil and registerrier Aug 14, 2018

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 14, 2018

Member

This is in response to #1633 , so @registerrier - you might also want to chime in.

@adonath - This is only for a spatial mask at the moment? Or does it work for a 3D mask or to select energy range as well?

Is applying the mask only to exposure OK? I guess cash is the only stat we use at the moment, and indeed I see that it is cash=0 if mu_on=0. But it might happen due e.g. to FFT noise from the PSF convolution that this results in slightly non-zero mu_on and weird stats effects? And what about background: it's a bug to just set exposure and npred from source counts to zero, but to keep positive background and n_on, no?

Maybe we could look to Sherpa for guidance and see what they do with masks passed via "filter" or "notice" calls?

Member

cdeil commented Aug 14, 2018

This is in response to #1633 , so @registerrier - you might also want to chime in.

@adonath - This is only for a spatial mask at the moment? Or does it work for a 3D mask or to select energy range as well?

Is applying the mask only to exposure OK? I guess cash is the only stat we use at the moment, and indeed I see that it is cash=0 if mu_on=0. But it might happen due e.g. to FFT noise from the PSF convolution that this results in slightly non-zero mu_on and weird stats effects? And what about background: it's a bug to just set exposure and npred from source counts to zero, but to keep positive background and n_on, no?

Maybe we could look to Sherpa for guidance and see what they do with masks passed via "filter" or "notice" calls?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Aug 14, 2018

Member

The downside is, that the exposure map is now different for MapFit and MapEvaluater. There the question whether MapEvaluater should take an exclasion mask as well. I was sure whether this makes sense, but maybe it does?

People are using the MapEvalutor to get model and residual maps. So I think we have to add the same mask option there as well.

Member

cdeil commented Aug 14, 2018

The downside is, that the exposure map is now different for MapFit and MapEvaluater. There the question whether MapEvaluater should take an exclasion mask as well. I was sure whether this makes sense, but maybe it does?

People are using the MapEvalutor to get model and residual maps. So I think we have to add the same mask option there as well.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Aug 15, 2018

Member

@cdeil The mask is ND now, so it can be used to filter the energy axis or other axes as well.

In general I agree this requires some more discussion, as it was not clear to me how to implement this best (and correct) either. So should the mask only be applied to the stat array? Or even only to the total_stat sum? Should maps such as exposure and background and npred be masked as well?
Yes, maybe it's a good idea to check how sherpa implements this.

Member

adonath commented Aug 15, 2018

@cdeil The mask is ND now, so it can be used to filter the energy axis or other axes as well.

In general I agree this requires some more discussion, as it was not clear to me how to implement this best (and correct) either. So should the mask only be applied to the stat array? Or even only to the total_stat sum? Should maps such as exposure and background and npred be masked as well?
Yes, maybe it's a good idea to check how sherpa implements this.

@adonath adonath merged commit e05ed80 into gammapy:master Aug 15, 2018

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

Map analysis automation moved this from To do to Done Aug 15, 2018

@cdeil cdeil changed the title from Add exclusion mask handling to MapFit to Add mask filter option to MapFit Sep 9, 2018

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