Duplicate the irf values for EFFAREA and EDISP when one wobble in stored irf

VERITAS-Observatory / V2DL3

VERITAS (VEGAS and Eventdisplay) to DL3 Converter

BSD 3-Clause "New" or "Revised" License

8 stars 1 forks source link

Duplicate the irf values for EFFAREA and EDISP when one wobble in stored irf #139

Closed sona-patel closed 1 year ago

sona-patel commented 2 years ago

Addressing #87

GernotMaier commented 2 years ago

Could you explain again if you are addressing an issue in gammapy here or in V2DL3? Question is if we should ask gammapy to be able to handle these type of IRFs.

sona-patel commented 2 years ago

Actually issue is in V2DL3 while storing IRF in DL3 when only one wobble is present.

When only one wobble is there in stored IRF, we are duplicating the IRF data and axes for interpolation to work (here), but while storing in DL3, it is storing only for one wobble (here). This gives rise to ValuErrror in Gammapy of mismatch data and axes shape.

ValueError: data shape (39, 1) does not matchaxes shape (39, 2).

This PR resolves this error.

GernotMaier commented 2 years ago

But the error is in gammapy and not in V2DL3?

sona-patel commented 2 years ago

No, error occurs in Gammapy because of incorrect DL3 file, so its issue in V2DL3.

GernotMaier commented 2 years ago

So where in the DL3 data standard is this required? I don't see that in the gamma-ray data format.

sona-patel commented 2 years ago

Isn't it rule of thumb that there should be one to one corresponds between axes and data? Do we need to go to DL3 data standard for this?

GernotMaier commented 2 years ago

We need to follow the standard by the letter, anything else will lead to problems in the future.

If gammapy does not follow the standard, then we need to ask for a change - but please check for the gamma-ray format page if there are any requirements on the number of bins given per axis. We definitely shouldn't put work arounds into the code for issues which should be solved some where else (we could add it temporarily, but then it should be clearly indicated)

GernotMaier commented 1 year ago

Let's follow this up, is this a correct description of the issue?

V2DL3 is providing IRFs with all axes included. The field of view axis (THETA_LO, THETA_HI) has a single bin only.
gammapy requires that axes have at least two bins

If this is the case, then I think it is a gammapy issue. There is nowhere in the GADF standard a requirement that axes need to have more than one bin.

sona-patel commented 1 year ago

Let's follow this up, is this a correct description of the issue?

V2DL3 is providing IRFs with all axes included. The field of view axis (THETA_LO, THETA_HI) has a single bin only.

gammapy requires that axes have at least two bins

If this is the case, then I think it is a gammapy issue. There is nowhere in the GADF standard a requirement that axes need to have more than one bin.

Yes this is the correct description of the issue!

GernotMaier commented 1 year ago

I am accepting this now, although it is clearly a fudge. Added a warning when this is used and raise an error not if this is used for full-enclosure IRFs.

We need to find a better solution for the future, IRFs in gammapy seem to have the functionality to have no offset dependency. To not forget it, I am opening the issue #168

sona-patel commented 1 year ago

Checked on rhv run 70323 with gammapy-0.20.1 - works without error!