Move LSTGeometry repo into ESProducer and standalone binary#203
Move LSTGeometry repo into ESProducer and standalone binary#203
Conversation
|
I expect that the final code will not use a csv file to generate the geometry bin/txt file. |
| std::vector<Polygon> difference; | ||
| for (auto &ref_polygon_piece : ref_polygon) { | ||
| std::vector<Polygon> tmp_difference; | ||
| boost::geometry::difference(ref_polygon_piece, tar_polygon, tmp_difference); |
There was a problem hiding this comment.
@slava77 currently the bottleneck is with polygon operations like this one. I'm using boost::geometry, but maybe you have a suggestion for another library. Currently, I'm having some issues with the ordering of points ending up creating self-intersecting polygons or negative areas.
There was a problem hiding this comment.
Is this really the slowest part? I would think that the straight line tracing should be the fastest.
is it going to be faster to add all polygons from list_of_detids_etaphi_layer_tar and then subtract them from ref_detid polygon?
here when considering new_tar_detids_to_be_considered and in the tar_detids_to_be_considered loop I'd expect that it would be much faster to place detids in eta-phi bins in each layer with some coarse binning (perhaps 32x32 or so) and then loop only over these eta-phi bins instead over all detids in a layer (access det_geom.getEndcapLayerDetIds(layer, etaphiBin) )
For the endcap, are the layers here for the positive and negative endcaps different or the same? If the same, then it sounds like an obvious speedup should be to skip the wrong side. The eta-phi binning will address this automatically
There was a problem hiding this comment.
I didn't mean this point in particular, just that the polygone operations through the code take up most of the time.
Yeah, binning sounds like the way to go. And for the endcap, I think the layer includes both, so that was another wasteful thing.
| double xnew = x * std::cos(-refphi) - y * std::sin(-refphi); | ||
| double ynew = x * std::sin(-refphi) + y * std::cos(-refphi); | ||
| x = xnew; | ||
| y = ynew; | ||
| } | ||
| double phi = std::atan2(y, x); | ||
| double eta = std::copysign(-std::log(std::tan(std::atan(std::sqrt(x * x + y * y) / std::abs(z)) / 2.)), z); |
There was a problem hiding this comment.
I'm not sure why this sincos rotation to xnew,ynew is done.
why is the phi here not simply atan2(y,x)-refphi ?
It would be even faster if phi for all corners is precomputed.
Also, eta doesn't depend on refphi, perhaps it's easier to precompute eta for all the corners and use the values directly (including the zshifts of 0, ±10).
There was a problem hiding this comment.
I'm not sure, I took it directly from here, but I'll think about it.
And yeah, pre-computing things would be good
|
|
||
| for (int zshift : {0, 10, -10}) { | ||
| std::vector<Polygon> ref_polygon; | ||
| ref_polygon.push_back(getEtaPhiPolygon(det_geom.getCorners(ref_detid), refphi, zshift)); |
There was a problem hiding this comment.
We probably don't really need polygons (and boost). I'd treat modules as rectangles using the larger side (xyz rectangles are wider in phi on the lower r side) for the positive overlap tests and the narrower side for the subtractive overlaps.
The simplest subtractive overlap can be done by breaking a module in e.g. 10x10 bins and just brute force checking each bin if it's covered or not to get to the endcap modules not covered by barrel.
A more precise and perhaps faster is to collect (ordered) etas and phis from the target modules and bin the reference module in rectangles along these etas and phis.
There was a problem hiding this comment.
Yeah, it would be ideal to ditch polygons and just work with a simplified approximation. I'll work on this.
|
After binning, it takes about 15 seconds, which is starting to be reasonable. There are more improvements that can be made to make it faster, but for now I'll switch gears to get the CMSSW part in place. |
ariostas
force-pushed
the
ariostas/lst_geometry
branch
from
c2d0f53 to
a5accdb
Compare
|
Not sure if it will work, but let's see what happens. /run cmssw |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
1 similar comment
|
There was a problem while building and running with CMSSW. The logs can be found here. |
ariostas
force-pushed
the
ariostas/lst_geometry
branch
from
a91bad2 to
14afa6a
Compare
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
The PR was built and ran successfully with CMSSW. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
The plots actually look better than I expected. I'll take another look at the standalone comparison and post it here. I'll also do some cleanup and look into what could be the source of the differences in the |eta|~1-2 range. |
|
/run cmssw |
There was a problem hiding this comment.
Pull request overview
This PR refactors the LST (Line Segment Tracking) geometry handling by moving geometry computation logic into a dedicated structure and making it available through both an ESProducer (for CMSSW integration) and a standalone binary. The key changes include:
- Introduction of a new
LSTGeometrydata structure containing centroids, slopes, pixel maps, and module connections - New geometry computation methods using Eigen for matrix operations
- An ESProducer (
LSTGeometryESProducer) that generates geometry fromTrackerGeometry - A standalone binary (
lst_make_geometry) for offline geometry generation - Integration with existing LST code through new overloaded methods
Key Changes
- New geometry computation framework with multiple helper classes (Module, DetectorGeometry, etc.)
- Refactored
LSTESDatato accept both file-based andLSTGeometry-based initialization - Updated parameter handling to use string-based
ptCutlabels for ES record lookups
Reviewed changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
RecoTracker/LSTCore/plugins/LSTGeometryESProducer.cc |
New ESProducer for generating LSTGeometry from TrackerGeometry |
RecoTracker/LSTCore/standalone/bin/lst_make_geometry.cc |
New standalone binary for geometry generation |
RecoTracker/LSTCore/interface/LSTGeometry/*.h |
New geometry data structures and computation methods |
RecoTracker/LSTCore/src/LSTESData.cc |
Added overload for LSTGeometry-based initialization |
RecoTracker/LSTCore/src/TiltedGeometry.cc |
Added overload to load from LSTGeometry slopes |
RecoTracker/LSTCore/src/EndcapGeometry.cc |
Added overload to load from LSTGeometry slopes |
RecoTracker/LSTCore/test/DumpLSTGeometry.cc |
New test analyzer for dumping geometry |
RecoTracker/LST/plugins/alpaka/LSTProducer.cc |
Updated ptCut parameter handling |
RecoTracker/LSTCore/standalone/Makefile |
Added build rules for geometry binary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
is the copilot reviewer feature something to be manually added? I don't see this option in #212 |
|
The PR was built and ran successfully with CMSSW. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
/run cmssw |
|
The PR was built and ran successfully with CMSSW. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
/run cmssw lowpt |
|
/run cmssw |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
The PR was built and ran successfully with CMSSW. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
run-ci: cmssw |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
run-ci: cmssw |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
I think I fixed the issues. I'll do one last big change that has the potential of breaking things again, so I'll run the CI beforehand. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
run-ci: cmssw |
|
The PR was built and ran successfully with CMSSW running on CPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
This is still a work in progress, but I'm opening a draft PR so you can track the progress.
This is relatively close to working for standalone, but I still have to figure out the ESProducer part.