cta-observatory / pyirf

Python IRF builder
https://pyirf.readthedocs.io/en/stable/
MIT License
15 stars 25 forks source link

IRF classes #151

Open maxnoe opened 3 years ago

maxnoe commented 3 years ago

Right now, we do not have special datastructures for IRFs, we use astropy quantities, numpy arrays.

This makes functions simpler and more flexible but an advantage of a specific datastructure would be to keep information together that belongs together (irfs with their bins).

Since gammapy already has these classes, we could think about using them, but that would again mean adding gammapy as required depedency.

An alternative would be to use some thin data class that holds the values, binnings and metadata.

kosack commented 3 years ago

I'd suggest a light internal data structure, since the gammapy's IRF structure may not be stable and it has some assumptions on how IRFs are dimensioned that we might not want to make initially in pyirf. For example, they assume IRFs are 2D or 3D and are already marginalized to a specific observation (which is fine for a DL3 IRF), but in pyirf might want to be able to make IRFs as a function of not only fov coordinates + energy, but also zenith angle, or other dimensions (a master IRF that can then be later marginalized to many observations).

kosack commented 3 years ago

could look at xarray for inspiration for the API at least. It may be overkill as an explicit dependency, though it does give you interpolation, plotting, and IO (to hdf5 netcdf) built-in (http://xarray.pydata.org/en/stable/data-structures.html).

maxnoe commented 3 years ago

The idea is actually pretty similar to the gammapy Maps and MapAxis classes, which are used for the IRF classes internally. We could use those with extra dimensions.

adonath commented 3 years ago

@maxnoe and @kosack In Gammapy we have (for "historical reasons") two nd data structures: IRF and Map, the Map class requires a sky coordinate axis first such as WCS, HEALPix or a region for spectra and has additional physical coordinate axes, the IRF class has just the physical coordinates axis. The assumption for both is that the physical coordinate axes are independent.

This year we spend some time to refactor the existing IRF classes in Gammapy and introduced a ND base class. The class takes the declaration of the axis coordinates as a list of MapAxis objects, data, unit and meta data. It offers e.g. interpolation, integration, cumsum, normalisation methods etc. all supported along specified axis names. In general the mid-term plan for Gammapy is to further unify the current IRF class and the Map class. So that other methods such as .upsample(), .downsample(), resample_axis() are supported as well for the IRF class (currently only supported for Map...) and can possibly moved to a common base class.

Using this base class the actual implementations of the IRF classes with fixed dimensions simplify to just declaring the physical axes and order and implement specific plotting functionality see e.g. EffectiveAreaTable2D. We also decided to keep the previously existing public API, that's why the base classes are not yet publicly documented.

In general I think a package like xarray is a good choice for such data structures however currently there are some limitations, which makes it non-ideal for our use-cases. Here are a few additional thoughts I have on this:

kosack commented 3 years ago

@adonath If you plan to move IRFs to use Maps, then that sounds like a good solution. It solves the dimensionality problem (assuming the spatial coordinates can be in AltAZ) and would be a nice interface. So I would suggest we just start with using gammapy.Maps. The only reason I originally hesitated to suggest that option was the uncertainty in the Science Tools selection, but now it's clear having gammapy as a dependency is not a big issue.