PIG 25 - Unbinned analysis

[tunbehaun273]

Description

This pull request will add a pig for the unbinned analysis

Dear reviewer

[codecov]

Codecov Report

Merging #4253 (4a9bd73) into main (cf3166f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 4a9bd73 differs from pull request most recent head 878605a. Consider uploading reports for the commit 878605a to get more accurate results

@@            Coverage Diff             @@
##             main    #4253      +/-   ##
==========================================
- Coverage   94.93%   94.92%   -0.01%     
==========================================
  Files         218      218              
  Lines       30888    30880       -8     
==========================================
- Hits        29322    29314       -8     
  Misses       1566     1566              

Impacted Files	Coverage Δ
gammapy/modeling/models/temporal.py	96.58% <100.00%> (ø)
gammapy/modeling/models/tests/test_temporal.py	99.67% <100.00%> (-0.02%)	⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[adonath]

A few notes from the discussion today:

• Prototype is available here https://github.com/gammapy/gammapy-unbinned-analysis/tree/main/EventDataset
• In the current implementation the EventDataset is very similar to the MapDataset, to avoid code duplication the PIG should sketch how the EventMapDataset could e.g. inherit from MapDataset:
  • Which methods would be supported? E.g. EventMapDataset.to_image(), EventMapDataset.cutout() etc.
  • Which methods would be different and how? E.g. EventMapDataset.npred(), also document return types, such as whether the method returns a np.ndarray, Quantity or maybe MapCoord
  • Which methods would just raise an error?
  • Is stacking supported?
  • For simplicity and precision we should just support the EDispMap and PSFMap and not the "kernel" version of those.
• The EventMapDataset could compute a counts maps "on the fly", this is convenient for visualization.
• Clarify whether we need a dedicated EventMapDatasetMaker (probably not!)
• The most complex code in the prototype is here and here. We should have an idea on how to simplify it and make it more re-usable. Here are some ideas:
  • The utility computes "per event" IRFs. It could make sense to introduce a separate abstraction for this, like EventPSF, or EventEDisp with an indexed MapAxis or LabelMapAxis to handle the event axis. But this might be tricky in many ways...
  • One place where we already use a per event IRF is in https://github.com/gammapy/gammapy/blob/main/gammapy/irf/psf/map.py#L285 and https://github.com/gammapy/gammapy/blob/main/gammapy/irf/edisp/map.py#L193, we should check if and how this could be re-used.
• The prototype introduce the concept of and "acceptance" map, which is different from what we have e.g. in MapDatasetOnOff.acceptance this should be resolved.
• I think the alternative describe an EventDataset with IRFs stored per event, but list the reasons why this is impractical (@tunbehaun273 has a lot of experience, which should be documented somewhere)
• @registerrier also raised the point pf whether the EventMapDataset is the more fundamental abstraction (to be clarified)
• @tunbehaun273 and @giacomodamico24 agreed to continue writing the PIG and the we will continue discussion in this PR.

[adonath]

Thanks @tunbehaun273 and @giacomodamico24 for the update! I think the only way to reduce the code duplication would be to move functionality down in the hierarchy to an un-binned data structure, basically an UnbinnedMap or, very similar, a SparseMap, but my current impression is that this is out of reach for this project as we are limited by the time you can provide for implementation. However I wanted to mention it for completeness.