MPGDTrackerDigi: Multi-SensitiveVolume solution for the 2D-strip read…

[ybedfer]

…out.

Briefly, what does this PR introduce?

A solution to the issue of MPGD hits missing in track fit raised by Barak Schmookler in "https://indico.bnl.gov/event/27589/contributions/105659/attachments/60986/104773/tracking1_040325.pdf".

What kind of change does this PR introduce?

• [*] Bug fix

Please check if this PR fulfills the following:

• [ *] Tests for the changes have been added

Does this PR introduce breaking changes? What changes might users need to make to their code?

Breaking changes if and only if special "-PMPGD:SiFactoryPattern" option is specified and set != 3.

Does this PR change default behavior?

Yes: slightly modified uncertainties for "OuterMPGDBarrel".

[ybedfer]

This is a solution to the missing MPGD 2D-strip hits which was identified in April.

The issue was discussed at the track&vertex meeting of July, see "https://indico.bnl.gov/event/29024/contributions/110567/attachments/63655/109264/Bedfer_EPIC0724.pdf", where several solutions were proposed.

In this PR, I want to implement a solution based on a splitting of the sensitive volume, see comment in "https://github.com/eic/EICrecon/blob/MultiSensitiveMPGD/src/algorithms/digi/MPGDTrackerDigi.cc".

It's the only solution, of all those proposed, that I have been able to implement. I would call it a ugly solution: very complicated. But it is has the advantage of being a working one, as evidenced by the following plot of the hit-to-track association efficiency obtained from (a modified version of) the macro of Barak:
hit_matching.PR.pdf

This result is obtained if and only if are used:

• A new version of the epic library, with modified .xml and .cpp files for the Barrel MPGDs.
• Option -PMPGD:SiFactoryPattern=0.
• A modified version of TrackerMeasurementFromHits (now included in the present PR).

Given that the above-mentioned conditions are not met by default, the commit will not affect the standard execution of eicrecon in any way.

The roadmap for the full commit, that will enable the Multi-SensitiveVolume solution:

• A new epic library, with Multi-SensitiveVolume XML files and backward-compatible detector constructors (in store: see "https://github.com/eic/epic/tree/Multi-SensitiveVolume-MPGD").
• A new version of TrackerMeasurementFromHits (without which the outer barrel efficiency is null), also backward compatible (now included in the present PR).
• If all goes fine, one would have to make Multi-SensitiveVolume the default, implying simultaneous changes to epic and eicrecon ( -PMPGD:SiFactoryPattern=0).

But in any case, I would like to have a feed-back from the ePIC software community on my solution: is it satisfying enough to be committed (at least temporarily)? What kind of checks, in addition to the association efficiency shown above, should I perform?

More on the hit-to-track association: When one zooms on the efficiency histogram, some imperfections show up, viz.:

• Fewer 'p' hits than 'n' hits, particularly for the inner barrel.
• Some inefficiency.
• Some outliers.
  See the following plot:
  hit_matching.PR.zoom.pdf
• In fact, 'p' and 'n' hits need not be in equal number: they may depart in the accumulation step, whereby hits falling on a same cellID are lumped together. That it is the case more often for 'p' than 'n' in the inner barrel can be traced to the fact that there, the two coordinates are very different (orientation along φ or Z). In the outer barrel, the two coordinates are symmetric and they should be in equal numbers. But there, the unbalance is small and may be due to chance.
• I would still have to investigate the inefficiency and the outliers...

[ybedfer]

A question to c++ experts:
One of the compilation checks launched by the PR returns the following warning:

/home/runner/work/EICrecon/EICrecon/src/algorithms/digi/MPGDTrackerDigi.cc:288:24: error: possibly dangling 
reference to a temporary [-Werror=dangling-reference]
  288 |     const TGeoHMatrix& toRefVol = refVol.nominal().worldTransformation();
      |                        ^~~~~~~~
/home/runner/work/EICrecon/EICrecon/src/algorithms/digi/MPGDTrackerDigi.cc:288:71: note: the temporary was 
destroyed at the end of the full expression 'refVol.dd4hep::DetElement::nominal().dd4hep::Alignment::worldTransformation()'
  288 |     const TGeoHMatrix& toRefVol = refVol.nominal().worldTransformation();
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

I did modify the source code (I take a copy of the matrix, instead of a reference) to get rid of the warning. But I'm wondering whether the warning itself is legitimate:

1. At execution time, everything seems to be fine.
2. I myself, do not see why the reference would be temporary.
3. I could read on the internet that this dangling reference warning produces many false positives.

What do you think?

[veprbl]

I think, the warning is a good one. In many ABIs your matrix is placed on stack and may get overwritten if you call something else before you use it.

(Haven't looked at the PR yet, this needs to be compilable on CI first)

[github-actions]

⚠️ Clang-Tidy found issue(s) with the introduced code (1/2)

[github-actions]

⚠️ Clang-Tidy found issue(s) with the introduced code (2/2)

[github-actions]

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

[github-actions]

Capybara summary for PR 2177

• rec_dis_10x100_minQ2=0_craterlake_10x100_MT
• rec_dis_10x100_minQ2=1000_craterlake_tracking_2DStrip
• rec_dis_10x100_minQ2=1000_craterlake_tracking_only
• rec_dis_18x275_minQ2=0_craterlake_18x275
• rec_dis_18x275_minQ2=0_craterlake_18x275_MT
• rec_dis_18x275_minQ2=0_craterlake_18x275_ST_MT
• rec_dis_18x275_minQ2=1000_craterlake_18x275
• rec_dis_18x275_minQ2=1000_craterlake_18x275_MT
• rec_dis_18x275_minQ2=1000_craterlake_18x275_ST_MT
• rec_dis_5x41_minQ2=0_craterlake_5x41
• rec_e_1GeV_20GeV_craterlake
• rec_pi_1GeV_20GeV_craterlake

Last updated 2026-03-08T18:27-04:00 89ab31f

[ybedfer]

I committed a few modifications, see "203e2f56".

• Addressing referee's comments.
• Changing marginally the behavior: no extension for reEntering particles (for particles on a reEntrance path, to be precise) and a fix to a mathematics mistake affecting the edge of cylinder volumes (a one in 10000 occurence). => No visible change to the results reported above.

(Note: I noticed that the source code had been changed in the mean time. But since I had been developing my own modifications and checked the changes do not affect the results, I decided to overwrite, not to have to do the checks all over again. Concerning the critical log level: I had understood that what needs to be changed are error messages from errors not raising an exception. This is what I have implemented: the errors leading to an exception are kept unchanged.)

[github-actions]

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

[ybedfer]

@veprbl
Hello,
It's more than two months that this PR is pending. It's too much.

• I need to move on. To the other PRs. (Multi sensitive mpgd clusters #2264 and Multi sensitive volume clusters epic#1021). To new developments including MPGD ECTs.
• At least, I need to have timelines.
• And this solution I am proposing for MPGD 2D-strip readout has to be understood as a temporary one. Implemented in order to be able to do urgent tasks, such as determining rates and impact of MPGD in a realistic setup. => It need not be perfect.

[veprbl]

Hello. This is a 2000 line PR, which is the main cause of delay. A promise was made by the tracking group to provide a review, but that did not happen. Request for timelines is well justifiable, my response is that I'm planning to finish this week.

[ybedfer]

Hello,
The point I wanted to stress (and I haven't stressed it enough in my previous post): since it's going to be temporary (at least as far as digitization, as opposed to hit reconstruction, is concerned), it need not be subjected to the same scrutiny as software pieces meant to be persistent. For example, whether string_view is used instead of strcmp does not matter. It does matter for what I call persistent software, in order to have a consistent C++ programming style, I agree. But in my particular case, it does not. => All checks concerning programming style and the like could be bypassed. => It would save time.

A remark concerning style checks (automatic ones, this time). It's the following layout modification, in https://github.com/eic/EICrecon/blob/MultiSensitiveMPGD/src/detectors/MPGD/MPGD.cc:

111,113c111,113
<     app->Add(new JOmniFactoryGeneratorT<MPGDTrackerDigi_factory>(
<         "MPGDBarrelRawHits", {"EventHeader", "MPGDBarrelHits"},
<         {"MPGDBarrelRawHits",
---
>     app->Add(new JOmniFactoryGeneratorT<MPGDTrackerDigi_factory>("MPGDBarrelRawHits",
>                                                                  {"EventHeader", "MPGDBarrelHits"},
>                                                                  {"MPGDBarrelRawHits",
115c115
< 	 "MPGDBarrelRawHitLinks",
---
>                                                                   "MPGDBarrelRawHitLinks",
117,123c117,123
< 	 "MPGDBarrelRawHitAssociations"},
< 	{
< 	  .readout   = "MPGDBarrelHits",
< 	  .threshold = 100 * dd4hep::eV,
< 	  .timeResolution = 10,
< 	},
< 	app));
---
>                                                                   "MPGDBarrelRawHitAssociations"},
>                                                                  {
>                                                                      .readout   = "MPGDBarrelHits",
>                                                                      .threshold = 100 * dd4hep::eV,
>                                                                      .timeResolution = 10,
>                                                                  },
>                                                                  app));

• It's ugly.
• It does not improve readability.
• It's not enforced in the very similar instructions further down the code.

=> There must be something wrong in the bot implementing this.

[veprbl]

Hello, these are good points to raise. Let me first clarify that basic coding changes like strcmp vs operator== are very minor matters that are not critical to the review process. Having said that, if we see minor defects (logging verbosity, use of C instead of C++), we might as well correct them as we go. The actual process behind the review is aimed at improved shared understanding among the collaboration and ensuring that there are no footguns for users and no issues during large-scale campaign productions. Review Team is currently working on documenting all aspects of the process as well as improving it, and, after it's shared in following months, hopefully, it will make clear what is being done.

About clang-format, the benefit of having a single automatic format is that it allows to not worry about variations in style. The current setting appears to have an edge case for those factory definitions, and perhaps there is an option that we could tweak to make it work better.