Skip to content

Conversation

@edcallaghan
Copy link
Contributor

@edcallaghan edcallaghan commented Apr 29, 2025

Offline data structures and conditions databases have historically labeled individual panels using pair of integers, enumerating planes in the tracker and panels within each plane, constituting a "mostly geographic" system; this is based on the nomenclature of DocDB-888. Each panel is read out by an individual "DTC link," which is a pair identifying DTCs within the computing system, and a channel within each DTC. The data payload from the tracker will label each hit by the panel's so-called ``Minnesota number,'' which is uncorrelated with both the geometry and readout channeling of the detector. Thus, data read out from the digi contain redundant panel-level information (the Minnesota number and DTC channeling), which must be translated into geographic coordinates for downstream analysis.

PR #1402 implements a deserialization routine for tracker digis and contains a fixed mapping from a few DTC channels to geographic coordinates. This PR generalizes that translation into a two-stage mapping derived from fcl: i) nominally, Minnesota numbers are translated into geographic coordinates, but; ii) an invalid Minnesota number in the data causes a second lookup in an optional table mapping DTC channeling and geographic coordinates (retaining a flag that the payload containing this digi is known to contain a mistake, i.e. the bad MN number). The Minnesota number is used as the primary key because it will be stored in persistent ROM on the frontend, and hence will not be susceptible to e.g. errors introduced by rearranging fibers/computers.

Try as I might, I could not construct a fhicl schema which would tolerate tables with keys not known at runtime, so all maps are written in fcl as tuples, from which maps are constructed in the deserialization module.

This PR should be merged after #1402 to preserve authorship and allow for a cleaner diff.

@FNALbuild
Copy link
Collaborator

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

  • RecoDataProducts
  • DAQ

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for 64c3cfe: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at 64c3cfe.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 64c3cfe at 90410d6
build (prof) Log file. Build time: 04 min 53 sec
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.
trigger Log file.
check_cmake Log file. Return Code 1.
FIXME, TODO TODO (0) FIXME (0) in 2 files
clang-tidy 🔶 20 errors 49 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 64c3cfe after being merged into the base branch at 90410d6.

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.

@eflumerf
Copy link
Contributor

@normanajn Should we discuss this in more detail? I think I was leaning more towards making this kind of map in the artdaq-core-mu2e data formats, as the other subsystems will also need to do this kind of HW-to-Digi channel mapping. Having it in configuration seems error-prone...

@FNALbuild
Copy link
Collaborator

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

@rlcee
Copy link
Collaborator

rlcee commented Apr 30, 2025

CRV and cal have this functionality implemented in the conditions service framework, like here. This code currently reads text files to define the conversion.

@edcallaghan
Copy link
Contributor Author

edcallaghan commented Apr 30, 2025

I didn't make a point of it in this PR, but the fcl interface, specifically, is only meant as a temporary measure to give us flexibility in decoding data from different test stands / tracker stations "today." We definitely will want something else as we approach having a single, static detector, and I (personally) don't currently have a preference between text-/db-based and/or moving the functionality into the data format codespace.

@normanajn
Copy link

normanajn commented Apr 30, 2025 via email

@brownd1978
Copy link
Collaborator

brownd1978 commented Apr 30, 2025

This PR addresses shortcomings of the original PR (1402). It is intended as a stopgap to allow detector work to continue while code development proceeds. I agree with the comments about long-term solutions, but those are outside the scope of this PR, which is to provide the algorithm in PR 1402 with code which meets Offline standards. I would appreciate your review from that perspective.

@brownd1978
Copy link
Collaborator

Note 2 tests failed in the CI. cmake is failing with:
File /scratch/workspace/GitHubPRTests/mu2e-offline-build-test/Offline/DAQ/src/StrawDigisFromArtdaqFragments_module.cc is not defined in CMakeLists.txt!
File /scratch/workspace/GitHubPRTests/mu2e-offline-build-test/Offline/DAQ/src/StrawRecoFromArtdaqFragments_module.cc defined in CMakeLists.txt but not found in directory!
Job completed with failure.

There are also 20+ clang-tidy errors associated with the use of Trace. Addressing those are beyond the scope of this PR, but it needs to be addressed eventually.

@edcallaghan
Copy link
Contributor Author

@FNALbuild run build test

@FNALbuild
Copy link
Collaborator

⌛ The following tests have been triggered for c5e945a: build (Build queue - API unavailable)

@FNALbuild
Copy link
Collaborator

☀️ The build tests passed at c5e945a.

Test Result Details
test with Command did not list any other PRs to include
merge Merged c5e945a at 3c0bbbc
build (prof) Log file. Build time: 04 min 35 sec
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.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 2 files
clang-tidy 🔶 20 errors 49 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at c5e945a after being merged into the base branch at 3c0bbbc.

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
Contributor

@eflumerf eflumerf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now, but an Issue should be opened to make sure that the channel-mapping is done in a way that satisfies all parties, and consistently across subsystems.

@brownd1978 brownd1978 merged commit f9eec53 into Mu2e:main May 2, 2025
14 checks passed
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.

7 participants