-
Notifications
You must be signed in to change notification settings - Fork 33
Smanthey/lowe flash matching update #171
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds functionality to match TPC clusters to PDS (Photon Detection System) flashes based on light map parametrized data. The implementation refactors flash matching logic to support both minimum and maximum PE (photoelectron) cuts using light map attenuation, and introduces a new flash selection method based on residuals.
Key Changes:
- Extracted light map configuration data into a dedicated
LightMapData.fclfile for better maintainability - Added new flash matching functions (
CutPDSFlashMinPE,CutPDSFlashMaxPE,SelectPDSFlash,GetLightMapParameters) to support light map-based flash selection - Extended configuration parameters to support separate minimum/maximum PE cuts and a configurable flash matching strategy
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| dunereco/LowEUtils/fcl/SolarEvent.fcl | Updated to include LightMapData.fcl and reference light map parameters from the new file; added FlashMatchBy configuration parameter |
| dunereco/LowEUtils/fcl/LightMapData.fcl | New file containing light map parametrization data for different detector geometries (dune10kt_1x2x6 and dunevd10kt_1x8x14_3view_30deg) |
| dunereco/LowEUtils/LowEUtils.h | Added declarations for new flash matching functions and member variables for maximum PE cuts, light map data, and flash matching method |
| dunereco/LowEUtils/LowEUtils.cc | Implemented new flash matching functions, refactored PE selection logic, added documentation comments, and reformatted function signatures for readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| continue; // Skip this flash if it's too far in time or in the future | ||
| } | ||
| if (int(flash->PEs().size()) < fAdjOpFlashMinNHitCut || *std::max_element(flash->PEs().begin(), flash->PEs().end()) / flash->TotalPE() > fAdjOpFlashMaxPERatioCut) { | ||
| continue; // Skip flashes with insufficient hits or too much concentration in one XA |
Copilot
AI
Jan 4, 2026
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.
Inconsistent indentation detected. Lines 1452-1457 use 10 spaces for indentation while the surrounding code uses 8 spaces (lines 1444-1450). This should be corrected to maintain consistent code formatting throughout the file.
| continue; // Skip this flash if it's too far in time or in the future | |
| } | |
| if (int(flash->PEs().size()) < fAdjOpFlashMinNHitCut || *std::max_element(flash->PEs().begin(), flash->PEs().end()) / flash->TotalPE() > fAdjOpFlashMaxPERatioCut) { | |
| continue; // Skip flashes with insufficient hits or too much concentration in one XA | |
| continue; // Skip this flash if it's too far in time or in the future | |
| } | |
| if (int(flash->PEs().size()) < fAdjOpFlashMinNHitCut || *std::max_element(flash->PEs().begin(), flash->PEs().end()) / flash->TotalPE() > fAdjOpFlashMaxPERatioCut) { | |
| continue; // Skip flashes with insufficient hits or too much concentration in one XA |
dunereco/LowEUtils/LowEUtils.cc
Outdated
| } | ||
| else { | ||
| continue; // Unknown geometry, skip this matching | ||
| continue; // Unknown geometry, skip this matching |
Copilot
AI
Jan 4, 2026
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.
Inconsistent indentation detected. Line 1519 uses 10 spaces for indentation while line 1518 uses 8 spaces. This should be corrected to maintain consistent code formatting throughout the file.
| continue; // Unknown geometry, skip this matching | |
| continue; // Unknown geometry, skip this matching |
| // for (int i = 0; i < int(fAdjOpFlashMinPELightMap.size()); i++) { | ||
| // if (fAdjOpFlashMinPELightMap[i].first == "Amplitude") { | ||
| // a = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | ||
| // } | ||
| // else if (fAdjOpFlashMinPELightMap[i].first == "Attenuation") { | ||
| // b = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | ||
| // } | ||
| // else if (fAdjOpFlashMinPELightMap[i].first == "Correction") { | ||
| // c = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | ||
| // } | ||
| // } |
Copilot
AI
Jan 4, 2026
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.
There are large blocks of commented-out code (lines 1579-1589, 1631-1641) that appear to be the old implementation. This commented code should be removed to improve code maintainability and readability, as the functionality has been replaced by the GetLightMapParameters function call.
| // for (int i = 0; i < int(fAdjOpFlashMinPELightMap.size()); i++) { | |
| // if (fAdjOpFlashMinPELightMap[i].first == "Amplitude") { | |
| // a = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | |
| // } | |
| // else if (fAdjOpFlashMinPELightMap[i].first == "Attenuation") { | |
| // b = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | |
| // } | |
| // else if (fAdjOpFlashMinPELightMap[i].first == "Correction") { | |
| // c = fAdjOpFlashMinPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMinPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMinPELightMap[i].second[2]; | |
| // } | |
| // } |
| // for (int i = 0; i < int(fAdjOpFlashMaxPELightMap.size()); i++) { | ||
| // if (fAdjOpFlashMaxPELightMap[i].first == "Amplitude") { | ||
| // a = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | ||
| // } | ||
| // else if (fAdjOpFlashMaxPELightMap[i].first == "Attenuation") { | ||
| // b = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | ||
| // } | ||
| // else if (fAdjOpFlashMaxPELightMap[i].first == "Correction") { | ||
| // c = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | ||
| // } | ||
| // } |
Copilot
AI
Jan 4, 2026
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.
There are large blocks of commented-out code (lines 1631-1641) that appear to be the old implementation. This commented code should be removed to improve code maintainability and readability, as the functionality has been replaced by the GetLightMapParameters function call.
| // for (int i = 0; i < int(fAdjOpFlashMaxPELightMap.size()); i++) { | |
| // if (fAdjOpFlashMaxPELightMap[i].first == "Amplitude") { | |
| // a = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | |
| // } | |
| // else if (fAdjOpFlashMaxPELightMap[i].first == "Attenuation") { | |
| // b = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | |
| // } | |
| // else if (fAdjOpFlashMaxPELightMap[i].first == "Correction") { | |
| // c = fAdjOpFlashMaxPELightMap[i].second[0] * pow(ClusterCharge, 2) + fAdjOpFlashMaxPELightMap[i].second[1] * ClusterCharge + fAdjOpFlashMaxPELightMap[i].second[2]; | |
| // } | |
| // } |
Add functions to match TPC clusters to PDS flashes based on light map parametrised data.