Add asymmetric energy dispersion/migration support #300

luca-dib · 2025-04-22T14:00:58Z

This adds functions for calculating the asymmetric energy dispersion and migration matricies to pyirf.irf.energy_dispersion and creating FITS table HDUs for the asymmetric energy dispersion to pyirf.io.gadf.

The functions are separated into functions for a theta / phi and lon / lat FoV binning for the direct calculation of energy dispersion and migration and the HDUs:

energy_dispersion_asymmetric_polar
energy_dispersion_asymmetric_lonlat
energy_migration_matrix_asymmetric_polar
energy_migration_matrix_asymmetric_lonlat
create_energy_dispersion_asymmetric_polar_hdu
create_energy_dispersion_asymmetric_lonlat_hdu

Whereas the function for conversion from dispersion to migration energy_dispersion_to_migration_asymmetric does not require this.

- energy_migration_matrix_polar calculates the migration matrix with FoV binning in true_source_fov_offset and true_source_fov_position angle of the FoV - energy_migration_matrix_lonlat uses true_source_fov_longitude and true_source_fov_latitude

codecov · 2025-04-22T14:02:48Z

Codecov Report

Attention: Patch coverage is 97.35683% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.95%. Comparing base (2990605) to head (6225ee4).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
pyirf/io/tests/test_gadf.py	76.92%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #300      +/-   ##
==========================================
+ Coverage   95.60%   95.95%   +0.34%     
==========================================
  Files          62       65       +3     
  Lines        3278     3856     +578     
==========================================
+ Hits         3134     3700     +566     
- Misses        144      156      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kosack · 2025-04-22T15:33:48Z

pyirf/io/gadf.py

+    **header_cards,
+):
+    """
+    Create a fits binary table HDU in GADF format for the asymmetric energy dispersion with polar FoV coordinates.


I have a small worry about the terminology: Since it's not obvious that "asymmetric" here means "non-radially-symmetric", i.e. with an azimuthal dependence, it would be good to explain that better. Otherwise, i would think "asymmetric energy dispersion" is asymmetric in true energy, which it is, but that is not what we are talking about here. Maybe just use similar terminology for describing spatial models as gammapy: 1D vs 2D, i.e.:

1D-polar: model is radially symmetric, i.e. no azimuthal dependance, f(r)

2D-polar: model depends on radius and azimuth, f(r,phi)

2D-cartesian: projected (?) fov_lon and fov_lat dependance, f(dlon,dlat)

Then your functions become, e.g. create_energy_dispersion_2d_polar_hdu

Thanks for your input!
Yes, I can see there certainly is some ambiguity in just calling it "asymmetric" and I agree that there might be a better way to express this.
Gammapy does not use the "nD" terminology to describe just the spatial axes though (at least for the IRFs), but includes the energy axis as well, e.g.

EnergyDispersion2D axes: energy_true and offset, migra (This implies to me the migra axis is ignored. Is this intentional or an inconsistency?)

PSF3D axes: energy_true, offset and rad.

I definitely think we should be consistent in the naming convention and that there is probably value in also being consistent with gammapy in the future. Whatever way is decided on should be applied to the rest of the IRF package in my opinion, but the details warrant some discussion. For reference, in the non-radially symmetric effective area I also chose the "3D" naming convention, see #281.

The inconsistency is in calling PSF3D 3D.

I think the "ND" should refer to the dimensions in the phase space in which an IRF component is stored, excluding intrinsic axes of the IRF component itself.

GADF calls effective area and energy dispersion 2d (energy and offset). The migra axis for the dispersion is an intrinsic axis of the IRF.

But then GADF calls the the "2d" PSF PSF_TABLE and gammapy changed that to PSF3D for some reason. It should have been PSF2D

I think the "ND" should refer to the dimensions in the phase space in which an IRF component is stored, excluding intrinsic axes of the IRF component itself.

This also seems reasonable to me, however then the GADF PSF_TABLE breaks with this again, as you said of course.

I would propose that for this PR in accordance with the GADF I change the naming to "3D", however this leaves the PSF for which I would also soon want to create a PR. Is this worth an issue to discuss the naming etc.?

Yes, I agree with the dimensionality being the spatial+energy, so my comment above becomes just d+1.

But you are right... for PSF you really need to describe both the internal (source coordinate) and external (field-of-view coordinate) dimensionality to be fully unambiguous. Not sure the best way to name those... e.g. a model with a "2D" distribution of the PSF that changes with energy, radius and azimuth in the FOV: PSF2D_3D? 2DPSF3D?, PolarPSF3D (vs RadialPSF3D for the case where the PSF itself is only radial in source coordinates)?, . It gets ugly! @bkhelifi let's try to get this right for VODF...

kosack · 2025-04-22T15:39:17Z

pyirf/io/gadf.py

+        Bin edges in true energy
+    migration_bins: numpy.ndarray
+        Bin edges for the relative energy migration (``reco_energy / true_energy``)
+    fov_offset_bins: astropy.units.Quantity[angle]


Docstring doesn't match signature: cut and paste error?

bkhelifi · 2025-04-23T13:26:53Z

Once you have some example file, do not hesitate to send them to the Gammapy team in order to make some tests... except if you have already made them;)

maxnoe · 2025-06-02T15:30:05Z

docs/changes/300.feature.rst

+* ``energy_migration_matrix_asymmetric_lonlat``
+* ``energy_dispersion_to_migration_asymmetric``
+
+Also adds functions to create fits table HDUs in GADF format for the energy dispersion to ``pyirf.io.gadf``.


Technically, this is not GADF as GADF does not have these.

Maybe write "similar to existing GADF defintions"?

maxnoe · 2025-06-02T15:38:14Z

pyirf/io/gadf.py

+    header["HDUCLAS4"] = "EDISP_3D"
+    header["DATE"] = Time.now().utc.iso
+    idx = edisp.colnames.index("MATRIX") + 1
+    header[f"CREF{idx}"] = "(ENERG_LO:ENERG_HI,MIGRA_LO:MIGRA_HI,DETX_LO:DETX_HI,DETY_LO:DETY_HI)"


I'd put the migra axis last, see open-gamma-ray-astro/gamma-astro-data-formats#178

I see the point, however in that case for consistency's sake the same should be applied to the 2D energy dispersion as well in my opinion. After checking this causes problems with the current tests for IO, specifically with the test_cref function. Will try to resolve these.

I agree in principle, but we are in a different situation with the 2d energy dispersion: we have it in GADF with the preferred axis order defined, so we should follow it. That this order is not the one we would prefer is an issue for GADF.

The 3D is new, so we should do what we think is most appropriate.

maxnoe · 2025-06-02T15:39:33Z

pyirf/irf/energy_dispersion.py

+        np.column_stack(
+            [
+                selected_events["true_energy"].to_value(u.TeV),
+                mu,


See comment above about axis order.

Please put the migra axis last. In general, the IRF intrinsic axes should come after the "parameter" axes.

Axis order for 3D EDISP is now changed to put the intrinsic axis last. Relevant tests updated accordingly. 3D EDISP is no longer wrongly referred to as part of GADF definitions.

luca-dib added 17 commits January 20, 2025 17:41

Add asymmetric edisp functions

e3c6dd2

Asymmetric edisp, support in normalization func

76fe4ce

Add tests for asymmetric edisp

f822327

Minor style changes

4bfd297

Add HDU output for asymmetric edisp

308a7e9

Add asymmetric edisp functions

9a2fb59

Add missing imports

b5bce60

Minor style changes

f473200

Add functions to __all__

df960ae

Add unit tests for asymmetric energy migration matrix

0076f26

Asymmetric dispersion to migration matrix function

fd49f7c

Add unit tests for asymmetric disp to migra

e4b7616

Fix typos and style changes

fbb2b0c

Minor style changes

67a67eb

Add newsfragment

45c2711

Make newsfragment more detailed

d8ca7b4

luca-dib requested review from HealthyPear and maxnoe as code owners April 22, 2025 14:00

Changed newsfragment to proper PR number

2b3a510

kosack reviewed Apr 22, 2025

View reviewed changes

kosack requested changes Apr 22, 2025

View reviewed changes

luca-dib and others added 4 commits April 28, 2025 16:30

Change "asymmetric" in function defs to "3d"

84856e2

Add unit tests for edisp_3d (xfail for now)

bed39e2

Style fixes

6225ee4

Merge branch 'cta-observatory:main' into edisp_asymmetric

d2c16d9

maxnoe reviewed Jun 2, 2025

View reviewed changes

luca-dib added the asymmetric label Jun 5, 2025

Address comments

b847bc4

Axis order for 3D EDISP is now changed to put the intrinsic axis last. Relevant tests updated accordingly. 3D EDISP is no longer wrongly referred to as part of GADF definitions.

Add asymmetric energy dispersion/migration support #300

Are you sure you want to change the base?

Add asymmetric energy dispersion/migration support #300

Uh oh!

Conversation

luca-dib commented Apr 22, 2025

Uh oh!

codecov bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kosack Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxnoe Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosack Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkhelifi commented Apr 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Apr 22, 2025 •

edited

Loading

kosack Apr 22, 2025 •

edited

Loading

maxnoe Apr 23, 2025 •

edited

Loading

kosack Apr 24, 2025 •

edited

Loading