Skip to content
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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kutschke
Copy link
Collaborator

@kutschke kutschke commented May 2, 2024

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,

  1. The RF marker TDC is set from the ProtonBunchTime data product
  2. The event length is set from the MakeEventHeaderFromLegacyMC_module.cc
  3. The onspill/offspill bit is set from the MakeEventHeaderFromLegacyMC_module.cc
  4. The remaining fields of EventHeader are zero initialized.

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.

@kutschke kutschke requested review from bonventre, brownd1978 and rlcee May 2, 2024 14:49
@FNALbuild
Copy link
Collaborator

Hi @kutschke,
You have proposed changes to files in these packages:

  • Print
  • Mu2eUtilities
  • CommonMC
  • GlobalConstantsService

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)

About FNALbuild. Code review on Mu2e/Offline.

@kutschke kutschke marked this pull request as draft May 2, 2024 14:53
@kutschke
Copy link
Collaborator Author

kutschke commented May 2, 2024

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.

@FNALbuild
Copy link
Collaborator

☔ The build is failing at 52355c3.

scons: *** [build/sl7-prof-e28-p056/Offline/tmp/Mu2eUtilities/src/EventHeaderFacade.os] Error 1
scons: *** [build/sl7-prof-e28-p056/Offline/tmp/CommonMC/src/MakeEventHeaderFromLegacyMC_module.os] Error 1
scons: *** [build/sl7-prof-e28-p056/Offline/tmp/Print/src/EventHeaderPrinter.os] Error 1
Test Result Details
test with Command did not list any other PRs to include
merge Merged 52355c3 at 64af1fc
build (prof) Log file.
ceSimReco 〰️ Log file.
g4test_03MT 〰️ Log file.
transportOnly 〰️ Log file.
POT 〰️ Log file.
g4study 〰️ Log file.
cosmicSimReco 〰️ Log file.
cosmicOffSpill 〰️ Log file.
ceSteps 〰️ Log file.
ceDigi 〰️ Log file.
muDauSteps 〰️ Log file.
ceMix 〰️ Log file.
rootOverlaps 〰️ Log file.
g4surfaceCheck 〰️ Log file.
FIXME, TODO TODO (0) FIXME (0) in 8 files
clang-tidy 🔶 10 errors 392 warnings
whitespace check no whitespace errors found

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.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

Copy link
Collaborator

@rlcee rlcee left a 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.

@kutschke
Copy link
Collaborator Author

kutschke commented May 3, 2024

Thanks for the comments and suggestions.

  • 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?

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.

  • 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.
    I thought about that but the facade needs access to Services and don't want data products to access services. I know that some do but I would really like to fix that. The ones that do are one of the issues that are blocking us from moving to the new art.

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.

@FNALbuild
Copy link
Collaborator

📝 The HEAD of main has changed to f6fe4b1. Tests are now out of date.

@bonventre
Copy link
Contributor

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?).
Should we include smearing in the rfmTDC beyond the binning?

@kutschke
Copy link
Collaborator Author

kutschke commented May 6, 2024

The offset of the RF0 is in DAQConditions EventTiming, we could have the other relative timings here as well.

Please show me where that is used both on the sim and reco sides.

This says legacyMC, are you also planning on replacing EventWindowMarkerProducer (or having it produce eventheaders instead of pbtime?).
We should replace it, either by adding an option to the module or by making a new module. Eventually we can retire the old option and MakeEventHeaderFromLegacyMC_module.cc. I am not sure how this all interacts with the current round of sim campaigns.

Should we include smearing in the rfmTDC beyond the binning?
When we compute rfmTDC from the analog signal it is truncated and when we restore the analog value we add back half a tick. So that adds (tick/sqrt(12) smearing already. There is a separate issue of what smearing should be done to the MC true value of the offset discussed in your first sentence.

@bonventre
Copy link
Contributor

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?

@kutschke
Copy link
Collaborator Author

kutschke commented May 6, 2024

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

@kutschke
Copy link
Collaborator Author

CI is not auto running on another PR. Let's see if this runs.

@FNALbuild run build test

@FNALbuild
Copy link
Collaborator

⌛ The following tests have been triggered for 52355c3: build (Build queue has 1 jobs)

@brownd1978
Copy link
Collaborator

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.
I suggest we do the minimum necessary now to test EventHeader without disrupting existing workflows.
Is there a plan to introduce into this PR any translation codes, or would that be a separate PR?

@kutschke
Copy link
Collaborator Author

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.

@FNALbuild
Copy link
Collaborator

❓ 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.

@FNALbuild
Copy link
Collaborator

📝 The HEAD of main has changed to 79ed54a. Tests are now out of date.

@brownd1978 brownd1978 self-assigned this May 31, 2024
Copy link
Collaborator

@brownd1978 brownd1978 left a 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

@kutschke kutschke mentioned this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants