ANNIEsoft / ToolAnalysis

Other
10 stars 53 forks source link

Official EventBuilding ToolChain #253

Closed S81D closed 8 months ago

S81D commented 1 year ago

see commit info, but here is a brief description of the changes (mostly changes to DataDecoder and other toolchains' config files)

To do:

marc1uk commented 10 months ago

Sweet! These all sound like great changes. And CI seems to have run successfully for this one, which is also good news. :tada:

I see here the dreaded words DaylightSavings and DaylightSavingsSpring .... I thought in one of your previous PRs we removed dependence on DST timestamps? What is this related to? Is there a non-DST alternative? As a worst case scenario it might be nice to look at implementing DST correction within the code; if we know the timestamp has a DST offset, and assuming we have a corresponding date too, we should be able to determine what the offset required is.

very minor point, but technically comments should be marked with # rather than //. This probably works because the Store parser will read the first non-whitespace word as a key and the second as a value and then stop there, but lines "commented out" with a preceding // will not be ignored, and it's probably misleading to have inconsistent comment styling for leading and trailing comments.

Another minor comment is that it's generally best not to commit high verbosity levels, as excessive printouts can slow down processing considerably.

As for the outstanding issues, I won't let them hold this up, but we have an update on those? @rory42edwards have you fixed your segfault? I think considering that the Tool was written, it would be good to have that merged, even if it immediately gets replaced. It's nice to have the progression and it there as a fallback in case of future issues.

S81D commented 10 months ago

Thanks for the comments - I will talk to Yue and Rory about the LAPPD related action items (for Yue, adding tools to do the time matching, for Rory fixing the segfault). As for DST, I can add a separate PR modifying the BeamFetcherConfig file to remove it (#233 , since the tool was modified to remove dependence), and look into altering the MRDDataDecoder tool to no longer rely on DST.

Only question I have is that I was under the impression that the MRD records timestamps in CDT or some other daylight savings dependent time zone, so if we are trying to pair events with the CTC or Tank, how do we modify this correctly? For the old beamfetcher changes to fix DST dependencies, the SQL times used to be in local chicago time, and the tool then adjusted that time into UTC. SQL was then changed to UTC, and the shift was removed from the tool. If the MRD records timestamps in local Chicago time, it appears there is no reference for the MRD tool to use to convert to UTC, correct? Unless I'm missing something simple