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
| std::vector<Polygon> ref_polygon; | ||
| ref_polygon.push_back(getEtaPhiPolygon(det_geom.getCorners(ref_detid), refphi, zshift)); | ||
|
|
||
| // Check whether there is still significant non-zero area |
There was a problem hiding this comment.
it would help to add some more detailed description of what is done (it took me a while to stare at this code to figure out what's happening)
|
|
||
| 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. |
|
/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. |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
Hmm I'll have to make some tweaks to be able to test the low pt setup, so I'll wrap this up tomorrow. |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
2 similar comments
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
There was a problem while building and running with CMSSW. The logs can be found here. |
|
/run gpu-cmssw lowpt |
|
The PR was built and ran successfully with CMSSW (low pT setup) on GPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
@slava77 I don't think the lowpt setup is working properly. The plots look the same, so I'm wondering if it was even working before. These are the extra lines that the CI adds to the .py file: https://github.com/SegmentLinking/TrackLooper-actions/blob/cmssw/lowpt_mod.py . Do you know what might be missing? |
how is it applied? if [[ "$LOW_PT" == "true" ]]; then
sed '28r ../lowpt_mod.py' step3_RAW2DIGI_RECO_VALIDATION_DQM.py
fibut this just dumps the concatenated content of the two files to stdout, as you can see in the log perhaps |
|
Oops you're right. I missed the |
|
/run gpu-cmssw lowpt |
btw, why line 28, this seems very brittle: is it the end of the file or do you need to insert before something else? It would be more practical to add a customise function or even a procModifier in the main cmssw source code for the switch to 0.6 GeV, now that it's a multi-line effort. |
|
Yeah, it's very brittle. It would definitely be ideal to add a procModifier. I haven't done this before, but I'll look into how to do this. |
|
The PR was built and ran successfully with CMSSW (low pT setup) on GPU. Here are some plots. OOTB All Tracks
The full set of validation and comparison plots can be found here. |
|
/run gpu-cmssw lowpt |
|
There was a problem while building and running with CMSSW on GPU. The logs 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.