Update comment with current status of TS info

[morcuended]

I updated the comment explaining the current status of time-stamp info in the code itself. Now, the GPS-WRS-UCTS system is providing timestamps with 50 ns accuracy which are still stored in the ucts_time container/variable instead of gps_time.

I did not change this and the other two TS (TIB and Dragon -based) are still being kept since UCTS TS info does not stably arrive at the data stream yet. Sometimes these TS are still missing.

@rlopezcoto, what do you think? Should we still keep the three TS or rename the ucts_time as gps_time already?

[rlopezcoto]

Thanks @morcuended
I agree with the renaming gps_time = ucts_time. To keep track, I'll copy here the longer explanation you gave when this was discussed by mail, feel free to modify/add anything if needed:
From @morcuended: "gps_time was the variable/container first defined in the ctapipe framework for storing the event timestamp (as it was defined for MC data). I wrote that comment at the time GPS was bypassed several months ago. Then we decided to use the "three different timestamp" i.e. UCTS, TIB and Dragon -based (the last two just for crosschecking). Hence we decided to name them explicitly ucts_time, tib_time and dragon_time. Now that GPS + UCTS system seems to be working fine, we should update that comment to avoid further confusion"

[morcuended]

I agree with the renaming gps_time = ucts_time.

Like this? ucts_time will be no longer a variable.

[rlopezcoto]

Now having a look at it... shouldn't we also update ctapipe-io-lst container name? ucts_timestamp -> gps_timestamp

[morcuended]

In lstchain/io/lstcontainers.py gps_time is already defined:

gps_time = Field(None, 'GPS time event trigger')
dragon_time = Field(None, 'Dragon time event trigger')
ucts_time = Field(None, 'UCTS time event trigger')
tib_time = Field(None, 'TIB time event trigger')

or you mean ucts_timestamp = Field(-1, "UCTS timestamp")
in ctapipe_io_lst/containers.py?

Edit: If it is the latter, I guess all these variable names containing 'ucts' are the ones that appear in the CameraEvent table itself.

[rlopezcoto]

Yeah, I was referring to the one in ctapipe-io-lst

[morcuended]

So you propose just to change ucts_timestamp = Field(-1, "UCTS timestamp") to gps_timestamp = Field(-1, "GPS timestamp")?

[rlopezcoto]

yes, this was my proposal, but maybe @FrancaCassol can comment on this, since she was the one who implemented this

[FrancaCassol]

Hi @morcuended and @rlopezcoto,
sorry, I am missing the point of this change. Why do you what to change ucts in gps?
The ucts_timestamp has this name because it is coming from the ucts structure.
At R0 level I think it is better that data names remain coherent with the EvB data.
Actually, the R0/R1 time of an event should be in the word "trigger_time" of the R0CameraContainer/R1CameraContainer containers. At the moment, I use the dragon time for setting it, but this should be changed with the ucts time as soon it is reliable
(generally speaking I would say that MC should adapt to data and not viceversa)

[morcuended]

Hi @morcuended and @rlopezcoto,
sorry, I am missing the point of this change. Why do you what to change ucts in gps?
The ucts_timestamp has this name because it is coming from the ucts structure.

I guess the idea was to explicitly reflect the fact that GPS is now working and providing (along with UCTS & WRS) TS.

At R0 level I think it is better that data names remain coherent with the EvB data.

This makes sense indeed.

Actually, the R0/R1 time of an event should be in the word "trigger_time" of the R0CameraContainer/R1CameraContainer containers. At the moment, I use the dragon time for setting it, but this should be changed with the ucts time as soon it is reliable

In my opinion, this might be already changed back to use ucts time instead of dragon-based TS. Of course, UCTS info sometimes does not reach EvB and is not written into the data stream...but there's nothing we can do about it.

(generally speaking I would say that MC should adapt to data and not viceversa)

Agree, but I guess that things have been done in this way since MC came first.

[FrancaCassol]

I guess the idea was to explicitly reflect the fact that GPS is now working and providing (along with UCTS & WRS) TS.

I think the UCTS time was always supposed to be a gps time (am I wrong?), so I would not change the software names because things now finally work ;-)

In my opinion, this might be already changed back to use ucts time instead of dragon-based TS. Of course, UCTS info sometimes does not reach EvB and is not written into the data stream...but there's nothing we can do about it.

I would prefer to change time when it will arrive in a stable way, better a less precise time that no time at all. Hopefully, this will happen soon

(generally speaking I would say that MC should adapt to data and not viceversa)
Agree, but I guess that things have been done in this way since MC came first.

But now data are there, so let's give them the priority

[morcuended]

OK, so for the moment I propose just changing the comment explaining the current status of time-stamp info and not touching anything else. What do you think, @FrancaCassol and @rlopezcoto?

[rlopezcoto]

Fine with me.

[FrancaCassol]

Fine also for me.