-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Populate EventHeader from EventWindowMarker and ProtonBunchTime #1252
base: main
Are you sure you want to change the base?
Conversation
…rmation found in the EventWindowMarker and ProtonBunchTime data products.
Hi @kutschke,
which require these tests: build. @Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main. ⌛ The following tests have been triggered for 52355c3: build (Build queue is empty) |
I converted to draft until the new artdaq_core_mu2e is available. I expect it later today or tomorrow. About reviews: Richie: can you look at the intellectual content of the EventHeader and the read facade class. In particular I truncate the time at creation of the RF0 TDC and I add back half a tick when I convert TDC to ns. I have adopted the convention that proton bunch time is always negative, which I believe is true by construction, but that the RF0 TDC is unsigned. This gives us a bigger dynamic range for the TDC. This will evolve as our understanding of the timing of the EventWindowMarker evolves. I presume that we will eventually need a conditions object to take out an overall offset. Ray: can you look at the GlobalConstants and Print infrastructure. Of course everyone is welcome to comment on everything but I would like to be sure that the specific items I mentioned are covered. |
☔ The build is failing at 52355c3.
N.B. These results were obtained from a build of this Pull Request at 52355c3 after being merged into the base branch at 64af1fc. For more information, please check the job page here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good, I have two general thoughts, I don't feel strongly, so I'm approving either way now
- the calculations like rfmTDC_estimated, which you note as "fixme" might be collected in a helper class called "timing" or similar where all the relationships can be documented together. I'm not sure if this is needed or wise, just a thought. I wonder if something like that will be needed for users reading data. Is there an accessible document on timing details?
- I think I would have put the facade in DataProducts (but not in genreflex) since it is a product, and is actually the one users should use, whereas the streamed one could be thought of as secondary to the "real" product.
Thanks for the comments and suggestions.
My picture of timing is that it's inherently factorized, each subsystem is responsible for it's internal timing. Then we need the detectors timed in relative to each other and then the ensemble timed in to the beam arrival. I think that the code should be organized that way. rfmTDC is the last item on this list so I think it stands on it's own. I do agree that within a subsystem all timing information should be collected together and that there should be one place to store the timing of all subsystems relative to each other. To the best of my knowledge the two documents I reference in the code are the full documentation about timing - it's pretty thin. One of the items on my plate is to writeup my understanding and iterate with Ryan and Richie until we agree. Then make it public. I guess it needs another step of circulating among us to catch missing details or descriptions that are ambiguous.
Eventually the facade will need to become Proditions aware. I bet that there will be offsets of the RF0 that change from year to year. Then we can decide if the facade uses a ProditionsEntity or becomes one. |
📝 The HEAD of |
The offset of the RF0 is in DAQConditions EventTiming, we could have the other relative timings here as well. This says legacyMC, are you also planning on replacing EventWindowMarkerProducer (or having it produce eventheaders instead of pbtime?). |
Please show me where that is used both on the sim and reco sides.
Should we include smearing in the rfmTDC beyond the binning? |
The timeFromProtonsToDRMarker is added to both pbtmc and pbt, there isn't a separate value for reco calibration since pbt represented the calibrated reco object, I guess if we're pulling directly from the eventheader we will need a separate calibration offset. What about things like ProtonBunchTimeFromStrawDigis_module? Are we assuming going forward we will always run reco off the eventheader timing? |
For data, I believe that we must run reco off of the EventHeader timing. I am doing this work now to see if we can update the sims so that the processing of simulated events to also use EventHeader timing, staring with MDC2024. The legacy bit is to see if we can do some development and testing with MDC2020 output. Does that work for you? I will look into the specific questions you asked and get back to you |
CI is not auto running on another PR. Let's see if this runs. @FNALbuild run build test |
⌛ The following tests have been triggered for 52355c3: build (Build queue has 1 jobs) |
To use this with existing sim data we first need code to translate existing objects (EWM, PBT, etc) into EventHeader. Then we can choose 2 ways forwards: we can write code to translate EventHeader into our existing objects and leave downstream codes as-is, or update them to use EventHeader directly. It's not clear to me which is better. |
As I understand it, the current code captures: https://mu2e-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=39805 . I don't believe that this is an accurate representation of what the actual plans are. I am working with Ryan to prepare a talk that describes the actual plans. I expect that the code will need to be touched in several places. Let me talk with RIchie and get back to you. |
❓ The build job(s) have failed or timed out on Jenkins, as there has been no result for over an hour. The tests may now be triggered again. |
📝 The HEAD of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any chance the conversion factors between tdc/clock ticks and ns could change? If not, this design is fine. If they could change, the standard dig -> hit paradigm would be be more appropriate, where the hit is the conversion of digital data into physical units. I'm particularly asking about
This PR will fail compilation in the CI until a new artdaq_core_mu2e is available, likely version v3_00_01 or higher.
The EventHeader class is found in artdaq_core_mu2e and, for data from the experimen t it, will be filled when the art::Event is created at the transition from artdaq to art. To prepare for data taking, there is a new module CommonMC/src/MakeEventHeaderFromLegacyMC_module.cc that creates a mu2e::EventHeader data product by reading information from the EventWindowMarker and ProtonBunchTime data products. Specifically,
This PR also provides Mu2eUtilities/inc/EventHeaderFacade.hh to convert TDC values and counters in the EventHeader data products back in to units of ns.
The GlobalConstants system was extended to support this work.
The Print system was extended to support printing the EventHeader.