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

Fix WcsNDMap and MapDataset cutout to support mode='partial' #2755

Merged
merged 2 commits into from
Jan 31, 2020

Conversation

registerrier
Copy link
Contributor

Description

This pull request solves issue #2754. First, an array of the correct size filled with 0 is created with the correct dtype. Then we add the content of the WcsNDMap using the the parenting cutout slices.
The cutout-slices are also used for the weight map that is used in WcsNDMap.

Note that I had put a test to ensure that other and weights have the same WcsGeom. This breaks many tests as it seems not to be the case in many situation. I found that puzzling. Maybe this should be clarified.

Dear reviewer

This is ready for review. We need to see whether mode='partial' should be the default for most observation loops. I tend to believe it should be since at least RingBackgroundMaker and FoVBrackgroundMaker need for field of view datasets to properly work.
Any opinion @adonath @AtreyeeS ?

I put v0.16 as milestone, and we'll see if we can really merge this in on time.

@registerrier registerrier added this to the 0.16 milestone Jan 30, 2020
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @registerrier, looks good to me. I have no further comments...

@adonath adonath changed the title Correct WcsNDMap and MapDataset cutout to support mode='partial' Fix WcsNDMap and MapDataset cutout to support mode='partial' Jan 31, 2020
Copy link
Member

@adonath adonath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @registerrier, I'll try to add the geom check in master and see if it can be easily resolved. If not we will keep it like it is...

@adonath adonath merged commit 1490909 into gammapy:master Jan 31, 2020
@adonath
Copy link
Member

adonath commented Jan 31, 2020

The "issue" is that the weights argument is sometimes provided as spatial map only (without energy axis). Internally we then rely on broadcasting to apply the weights. I think this behaviour is fine. In 81cdd1b I added at least a test for the spatial geometries...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants