Add new corrected geometry : version = 4

[FrancaCassol]

Geometry version 3 has a wrong PIX_ROT value. This is clearly visible if one makes a zoom of the pixels (see attached plots).
I created geometry version 4 which seems ok and changed the geometry version in the reader.

Should I also remove geometry 3 from the resources in order to avoid its use?

[vuillaut]

Hi.
Well spotted @FrancaCassol .
Why not directly updating the camera geometry 3 rather than creating a new one?

[FrancaCassol]

Hi @vuillaut,
for bookkeeping, people should recognise the wrong situation from the correct one.
I would say that this is the meaning of versioning: when something changes, the version changes...

[maxnoe]

Because versions should indicate that changes were made?
Granted, the pixel orientation is currently used only for displaying purposes, but in the future it could also be used for other stuff, e.g. for toy showers to calculate which pixel was hit by a photon.

[vuillaut]

Ha ok.
In this case, I was seeing versions just as different cameras that could have been named otherwise and saw this as a bug fix on a specific camera.
But actual versioning is fine :)

[FrancaCassol]

Ah, now I understand your question.
No, this is always the LST camera even if we indeed add the version in the CAM_ID name (which becomes in this case LSTCam-004).
I wonder, for keeping the code back compatible, should we perhaps avoid to hardcode the geometry version in the camera name? The version can anyway be found in the reader attribute event_source.geometry_version

[maxnoe]

I think TAB_VER does not refer to the version of the file, but to the format of the table in the file, see

https://github.com/cta-observatory/ctapipe/blob/01c715d3bad00d9a3c4b91f0cc6ac19acec5541b/ctapipe/instrument/camera.py#L305

[FrancaCassol]

Hi @maxnoe,

I think TAB_VER does not refer to the version of the file, but to the format of the table in the file,

I see, then should I perhaps add a CAM_VER meta data?

[kosack]

yes, CAM_VER would be good.

TAB_VER is indeed the "table Data model version" (probably it wasn't the best name). If you add CAM_VER, you should also then increase TAB_VER (since you change the data model).
Anyhow, there will be better headers defined by CTA very soon (working on releasing the doc right now) that will recommend better naming.

[rlopezcoto]

Thanks @FrancaCassol!
Since this is only affecting the display, I would say that we can make a bugfix release for ctapipe_io_lst that can be included in the next lstchain version, do you agree?

[FrancaCassol]

@rlopezcoto,
yes, I agree