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

2D-strip digitization for CyMBaL and Outer MPGDs #1695

Merged
merged 40 commits into from
Jan 20, 2025
Merged

2D-strip digitization for CyMBaL and Outer MPGDs #1695

merged 40 commits into from
Jan 20, 2025

Conversation

ybedfer
Copy link
Contributor

@ybedfer ybedfer commented Jan 7, 2025

Briefly, what does this PR introduce?

  • More realistic, 2D-strip readout of CyMBaL and Outer MPGDs.
  • This is done at the digitization level, elaborating on a new description of these detectors in epic (commit #efc19d119).
  • A run-time option (MPGD:SiFactoryPattern) allows to disable 2D-strip and go back to pixel readout on a per MPGD-detector basis.
  • If more realistic, the digitization is still simplistic: single-hit clusters.

What kind of change does this PR introduce?

  • Other: New feature.

Please check if this PR fulfills the following:

  • Changes have been communicated to collaborators

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

No.

Does this PR change default behavior?

Yes.

Residuals TrackerHit - SimTrackerHit

CyMBaL.Zphi.singleHit.pdf
OuterRes.UV.singleHit.pdf

@github-actions github-actions bot added topic: tracking Relates to tracking reconstruction topic: barrel topic: digitization labels Jan 7, 2025
@ybedfer ybedfer requested a review from wdconinc January 7, 2025 15:47
@ybedfer ybedfer self-assigned this Jan 7, 2025
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Thank you for your submission. I have a few comments and questions on this.

src/algorithms/digi/MPGDTrackerDigi.cc Outdated Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigi.h Outdated Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigi.h Outdated Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigiConfig.h Outdated Show resolved Hide resolved
src/factories/digi/MPGDTrackerDigi_factory.h Outdated Show resolved Hide resolved
src/detectors/MPGD/MPGD.cc Outdated Show resolved Hide resolved
src/detectors/MPGD/MPGD.cc Outdated Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigi.cc Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigi.cc Show resolved Hide resolved
src/algorithms/digi/MPGDTrackerDigi.cc Show resolved Hide resolved
@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 12, 2025

I set the default = no MPGDTrackerDigi_factory.
And let's try again.
Anyway I have absolutely no clue what can be the reason for the empty histos. RawTrackHit is as it should. The association with SimTrackerHit is also as it should. See the histos I put in the PR.

Does the seeding make use of MPGDs? If indeed, that would be the explanation.

@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 13, 2025

Then, could it be that:

  1. The seeding of tracks is based on MPGDs?
  2. The seeding algorithm is relying on MPGD hits being pixels and fails when it gets strips instead?

If so, we could agree to merge the 2D-strip-MPGDs branch as it is now: meaning

  • w/o MPGDTrackerDigi_factory by default, and then working,
  • w/ still the option -PMPGD:SiFactoryPattern=0, i.e. 2D-strip enabled, in order to investigate the seeding (could also be =0x2: 2D-strip in CyMBaL alone, or =0x1: 2D-strip in Outer alone).

@veprbl
Copy link
Member

veprbl commented Jan 14, 2025

Then, could it be that:

  1. The seeding of tracks is based on MPGDs?
  2. The seeding algorithm is relying on MPGD hits being pixels and fails when it gets strips instead?

I believe we don't have pixels or strips, just hits. For seeding, I would imagine, it does matter what are their positions. Is there supposed to be an additional algorithm to reconstruct intersections of strips?

cc @bschmookler

@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 15, 2025

Is there supposed to be an additional algorithm to reconstruct intersections of strips?

  • I don't know. But if the seeding hypothesis, i.e. seeding-in-MPGDs-fails-when-strips, is confirmed, it could be a way out.
  • What's striking is the 100% failure rate. => Something very basic must be missing.
  • Can you tell me which piece(s) of software are involved in the seeding? So that I can investigate. Can be that seeding is not the issue. But it's anyway the starting point, from where investigations can develop.

cc @bschmookler

@bschmookler
Copy link
Contributor

bschmookler commented Jan 15, 2025

The seeding algorithm takes in all the digitized tracker hits in global coordinates (CentralTrackingRecHits):

https://github.com/eic/EICrecon/blob/main/src/global/tracking/tracking.cc#L153-L159

It then only searches for seeds in the SVT volume in the barrel:

https://github.com/eic/EICrecon/blob/main/src/algorithms/tracking/OrthogonalTrackSeedingConfig.h#L17



Does the problem you are seeing also occur when you look at the truth-seeded tracking? In that case, the algorithm is only using the MC particle information to seed the Combinatorial Kalman Filter, rather that any tracking detector info.

@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 18, 2025

I cannot reproduce the problem.
I privately executed the tracking_performances_dis benchmark, taken from:

https://github.com/eic/detector_benchmarks/tree/master/benchmarks/tracking_performances_dis

successively w/ option MPGD:SiFactoryPattern set =0x3 (SiliconTrackerDigi) and = 0x0 (MPGDTrackerDigi).
=> No difference other than second order ones. See attached pdf.
MPGD.vs.Si.TrackerDigi.pdf

I am then going to switch back to MPGDTrackerDigi by default. So that the check is started again. This, presuming that the previously observed problem of empty histos was due to some glitch. Let's see...

If the problem happens again, the ideal would be to have in a next iteration a podio output of the benchmark including MPGD hits.

@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 19, 2025

I don't quite understand the reason for the failure of eicweb/eic_container.

  • There's an error concerning bench:lfhcal_campaigns: Job's log exceeded limit of 4194304 bytes. which seems to be unrelated to my PR.
  • Else a failure in bench:tracking_performance_campaigns which happens to be more in line w/ the empty-histos previous problem but seems to be due to file handling by the computer system:
Error in rule fetch_epic:
    jobid: 22
    output: EPIC/RECO/23.10.0/epic_craterlake/SINGLE/pi-/10GeV/130to177deg/pi-_10GeV_130to177deg.0000.eicrecon.tree.edm4eic.root
    shell:
        
xrdcp --debug 2 root://dtn-eic.jlab.org//work/eic2/EPIC/RECO/23.10.0/epic_craterlake/SINGLE/pi-/10GeV/130to177deg/pi-_10GeV_130to177deg.0000.eicrecon.tree.edm4eic.root EPIC/RECO/23.10.0/epic_craterlake/SINGLE/pi-/10GeV/130to177deg/pi-_10GeV_130to177deg.0000.eicrecon.tree.edm4eic.root
        (one of the commands exited with non-zero exit code; note that snakemake uses bash strict mode!)

@veprbl
Copy link
Member

veprbl commented Jan 19, 2025

The failures are known, and explained by a change to external states (new PyPi packages published, old campaigns deleted), that is to be fixed by eic/detector_benchmarks#128.

The latest tracking_perfromances_dis looks fine now
https://eicweb.phy.anl.gov/EIC/benchmarks/detector_benchmarks/-/jobs/4546314/artifacts/file/results/tracking_performances_dis/epic_craterlake_tracking_only/pythia8NCDIS_18x275_minQ2=1_combined_5/plots.pdf
which is what I can not explain yet. Let's assume that was a transient issue.

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

The strip re-encoding algorithm looks good to me. It's unfortunate that this is based on a copy of SiliconTrackerDigi instead of keeping digitization as a common separate algorithm. Test results look good to me.

@ybedfer
Copy link
Contributor Author

ybedfer commented Jan 20, 2025

keeping digitization as a common separate algorithm.

As is, SiliconTrackerDigi cannot serve for the digitization of MPGDs, because it's dealing w/ independent input hits, see e.g.:

    for (const auto& sim_hit : *sim_hits) {

        // time smearing
        double time_smearing = m_gauss();
        double result_time = sim_hit.getTime() + time_smearing;
        auto hit_time_stamp = (std::int32_t) (result_time * 1e3);

While MPGD hits are are correlated: a single ionization process, w/ its energy deposit and timing, gives rise to two hits.

@ybedfer ybedfer added this pull request to the merge queue Jan 20, 2025
Merged via the queue into main with commit 32070b4 Jan 20, 2025
85 of 86 checks passed
@ybedfer ybedfer deleted the 2D-strip-MPGDs branch January 20, 2025 14:08
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.

3 participants