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

PIG 2 - New low-level analysis code #1277

Merged
merged 5 commits into from Jul 27, 2018

Conversation

2 participants
@registerrier
Copy link
Contributor

registerrier commented Feb 1, 2018

This is provides a description of the proposed low level analysis scheme for image/cube analysis.

A description of the PSF kernels is still missing.

Please comment. @cdeil @adonath @bkhelifi @leajouvin

@registerrier registerrier requested a review from cdeil Feb 1, 2018

@cdeil cdeil self-assigned this Feb 1, 2018

@cdeil cdeil added the pig label Feb 1, 2018

@cdeil cdeil added this to the 0.8 milestone Feb 1, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 2, 2018

@registerrier - you currently have "PIG 2 - Organization of low-level analysis code" as title for the PIG and "Initial commit for the PIG 002" as title for the PR.

Can you please make them the same?

Do you plan to also outline 1-d spec analysis here, or only map-based analysis?
Maybe "PIG 2 - New low-level map-based analysis code"?

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 2, 2018

I saw a mention of "4D EDISP cube". Do we want to have spatially-dependent PSF / EDISP? Or only support spatially constant PSF / EDISP?

So far, in IACT, spatially dependent PSF / EDISP wasn't used. But for CTA with the higher precision it's probably needed at some point. There is no format spec yet, but ctools already has some FITS formats for that that we could probably just use if they are reasonable and in line with the maps formats that Matthew defined in the open spec. See ctpsfcube ctedispcube.

I think PSF / EDISP application in the likelihood fit would in many cases still be for a PSF kernel / EDISP matrix that isn't spatially varying, for simplicity and good speed, no? And spatially-dependent PSF / EDISP is something we would maybe implement separately later?

So @registerrier and all - what should we implement now, in the coming weeks? Is it extensible to the case of spatially-dependent PSF / EDISP in a backwards-compatible way (possibly by just adding new classes and functions for that case)? Or do we have to do the full design now?

@cdeil cdeil changed the title Initial commit for the PIG 002 PIG 2 - New low-level analysis code Feb 4, 2018

@registerrier

This comment has been minimized.

Copy link
Contributor

registerrier commented Feb 5, 2018

The mention of a 4D object EDISP is mostly because to actually compute exposure in reco energy we need to convolve Expo(X,Y,Etrue) with EDISP(Etrue, Ereco). This is done in practice through a 4D EDISP(X,Y,Etrue, Ereco) which will be transient in memory.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 5, 2018

This EDISP would probably dominate memory use. If it's not spatially dependent, then it should be possible to use Numpy broadcasting with a 2D EDISP and avoid the 4D. In the existing cube code from Lea, is it a 2D or 4D array? There's also the question with which maps EDISP works. Maybe just put these as questions in the document for now, and then we look at this in detail later this week?

@cdeil cdeil modified the milestones: 0.8, 0.9 May 7, 2018

@cdeil cdeil modified the milestones: 0.9, 0.8 Jul 15, 2018

@cdeil cdeil assigned registerrier and unassigned cdeil Jul 15, 2018

@cdeil cdeil added this to To do in Map analysis via automation Jul 15, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jul 15, 2018

@registerrier - Could you please finish up this PR?

I'd suggest to remove the code example file here, do minor text updates to reflect the fact that this PIG basically has been implemented in Gammapy in the past months (maybe pointing to the PR where you added the code to Gammapy) and then we merge this to have it on the record for the future.

IMO the goal is to continue the sprint on Gammapy this week and to have v0.8 ready by the end of the week and things cleaned up.

@registerrier

This comment has been minimized.

Copy link
Contributor

registerrier commented Jul 27, 2018

The python example file has been removed and a decision has been added with reference to PR #1314 which gives the first step implementing PIG 002.

@registerrier registerrier merged commit 64f31e0 into gammapy:master Jul 27, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Map analysis automation moved this from To do to Done Jul 27, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jul 27, 2018

@registerrier - thank you for all your work on this!

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Jul 31, 2018

I've done a little big of cleanup and clarified how the decision was done on PIG 2 in 77ad6a9.

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