Add TI time offset to conditions system

[JeremyMcCormick]

• Added ti_time_offsets table to conditions database

• Added TiTimeOffset conditions class and converter

• Created a simple test to read the TI time offset for a run

• Removed dependence of evio module on run database

• Read TI time offset from conditions system instead of run db in LCSimEngRunEventBuilder

• Removed now unnecessary setting of run db connection parameters from properties file (instead there is an existing constructor which can be used which takes a Connection object)

This was done primarily to remove the dependence of the EVIO to LCIO conversion on the run database and make this information easily accessible to reconstruction Driver classes.

[mholtrop]

Basic code changes look fine, but there are almost no comments!
Has this been tested on some actual data?

[JeremyMcCormick]

Has this been tested on some actual data?

I ran the test to make sure that the TI time offset for a test run was available but I did not run on any EVIO files (I'll test this when I get a chance).

there are almost no comments!

Changes to the EVIO converter seemed self-explanatory. I didn't add comments to the new conditions class but will do so.

[mholtrop]

Appears to be OK.
Some limited comments were added. Much of the code is fairly clear.

[omar-moreno]

It would have been good for @mrsolt to have checked that the correct latency was being read from the db. These are the type of subtle changes that may seem small, but can really mess up the reconstruction.

[JeremyMcCormick]

I've just checked run 5772 and it looks fine.

2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 11 with sequence 0
2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987067816
2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 12 with sequence 1
2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987094080
2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 13 with sequence 2
2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987110736
2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 14 with sequence 3
...

If you want to verify yourself then run something like this:

java -cp ~/.m2/repository/org/hps/hps-distribution/3.11-SNAPSHOT/hps-distribution-3.11-SNAPSHOT-bin.jar org.hps.evio.EvioToLcio \
    -dHPS-EngRun2015-Nominal-v5-0 \
    -DoutputFile=hps_5772_data ./hps_005772.evio.0

The changes from this PR don't actually do very much. The same numbers are being read as before, just from a different database.

[omar-moreno]

In my opinion, printing out the time doesn't actually constitute a test. It doesn't make sense to look at the TI time printout and conclude that nothing in the recon was impacted. Instead, you should have asked someone who knows how the SVT timing works to review your changes. For example, we could have simply looked at the distribution of hit times before and after the change was implemented in order to make sure things didn't change. I understand this is a 'simple' change, but changes like this have gotten us in trouble before. I would be especially cautious since we are preparing for pass1 of the 2016 data. Changes like this could have really broken the recon and caused us major headaches. In the future, make sure the subsystem experts know you are making these type of changes and follow the proper review process. On May 16, 2017 6:54 PM, "Jeremy McCormick" <notifications@github.com> wrote: I've just checked run 5772 and it looks fine. 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 11 with sequence 0 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987067816 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 12 with sequence 1 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987094080 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 13 with sequence 2 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987110736 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 14 with sequence 3 ... If you want to verify yourself then run something like this: java -cp ~/.m2/repository/org/hps/hps-distribution/3.11-SNAPSHOT/hps-distribution-3.11-SNAPSHOT-bin.jar org.hps.evio.EvioToLcio \ -dHPS-EngRun2015-Nominal-v5-0 \ -DoutputFile=hps_5772_data ./hps_005772.evio.0 The changes from this PR don't actually do very much aside from reading the same base TI time offset except from a different database. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADiWXL3FWv2_hX86adcEvxwQMggNcbnHks5r6lNbgaJpZM4NZ7o7> .

[mholtrop]

Hello Omar, I appreciate the carefulness, and I agree that such changes need to be checked. Is it sufficient in this case to print the TI data time before and after the code changes for a couple of events and verify that the number is identical? I would think that if it is, then the functionality of the code hasn’t changed. The concern could be whether the offset numbers are copied correctly from one DB to the other, but that should then be checked separately. Best, Maurik Before modifications: 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 22 with sequence 11 tiData.getTime() + tiTimeOffset = 1431853731987565192 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 23 with sequence 12 tiData.getTime() + tiTimeOffset = 1431853731987647816 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 24 with sequence 13 tiData.getTime() + tiTimeOffset = 1431853731987651164 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 25 with sequence 14 tiData.getTime() + tiTimeOffset = 1431853731987737260 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 26 with sequence 15 tiData.getTime() + tiTimeOffset = 1431853731987865788 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 27 with sequence 16 tiData.getTime() + tiTimeOffset = 1431853731987973188 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 28 with sequence 17 tiData.getTime() + tiTimeOffset = 1431853731988017160 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 29 with sequence 18 tiData.getTime() + tiTimeOffset = 1431853731988051384 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 30 with sequence 19 tiData.getTime() + tiTimeOffset = 1431853731988079432 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 31 with sequence 20 tiData.getTime() + tiTimeOffset = 1431853731988122908 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 32 with sequence 21 tiData.getTime() + tiTimeOffset = 1431853731988178512 2017-05-17 10:04:25 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 33 with sequence 22 tiData.getTime() + tiTimeOffset = 1431853731988204556 After ModificationsL 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 22 with sequence 11 tiData.getTime()+tiTimeOffeset = 1431853731987565192 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 23 with sequence 12 tiData.getTime()+tiTimeOffeset = 1431853731987647816 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 24 with sequence 13 tiData.getTime()+tiTimeOffeset = 1431853731987651164 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 25 with sequence 14 tiData.getTime()+tiTimeOffeset = 1431853731987737260 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 26 with sequence 15 tiData.getTime()+tiTimeOffeset = 1431853731987865788 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 27 with sequence 16 tiData.getTime()+tiTimeOffeset = 1431853731987973188 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 28 with sequence 17 tiData.getTime()+tiTimeOffeset = 1431853731988017160 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 29 with sequence 18 tiData.getTime()+tiTimeOffeset = 1431853731988051384 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 30 with sequence 19 tiData.getTime()+tiTimeOffeset = 1431853731988079432 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 31 with sequence 20 tiData.getTime()+tiTimeOffeset = 1431853731988122908 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 32 with sequence 21 tiData.getTime()+tiTimeOffeset = 1431853731988178512 2017-05-17 10:09:01 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 33 with sequence 22 tiData.getTime()+tiTimeOffeset = 1431853731988204556

…

On May 16, 2017, at 11:18 PM, Omar Moreno ***@***.***> wrote: In my opinion, printing out the time doesn't actually constitute a test. It doesn't make sense to look at the TI time printout and conclude that nothing in the recon was impacted. Instead, you should have asked someone who knows how the SVT timing works to review your changes. For example, we could have simply looked at the distribution of hit times before and after the change was implemented in order to make sure things didn't change. I understand this is a 'simple' change, but changes like this have gotten us in trouble before. I would be especially cautious since we are preparing for pass1 of the 2016 data. Changes like this could have really broken the recon and caused us major headaches. In the future, make sure the subsystem experts know you are making these type of changes and follow the proper review process. On May 16, 2017 6:54 PM, "Jeremy McCormick" ***@***.***> wrote: I've just checked run 5772 and it looks fine. 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 11 with sequence 0 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987067816 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 12 with sequence 1 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987094080 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 13 with sequence 2 2017-05-16 18:50:39 [INFO] org.hps.evio.LCSimEngRunEventBuilder getTime :: calculated TI data time = 1431853731987110736 2017-05-16 18:50:39 [INFO] org.lcsim.job.EventMarkerDriver process :: Event 14 with sequence 3 ... If you want to verify yourself then run something like this: java -cp ~/.m2/repository/org/hps/hps-distribution/3.11-SNAPSHOT/hps-distribution-3.11-SNAPSHOT-bin.jar org.hps.evio.EvioToLcio \ -dHPS-EngRun2015-Nominal-v5-0 \ -DoutputFile=hps_5772_data ./hps_005772.evio.0 The changes from this PR don't actually do very much aside from reading the same base TI time offset except from a different database. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADiWXL3FWv2_hX86adcEvxwQMggNcbnHks5r6lNbgaJpZM4NZ7o7> . — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEY_4kDe2EKdyVYiE86Oh1zF9vLJcLevks5r6mb2gaJpZM4NZ7o7>.