LST: add LSTGeometry package and associated ESProducer#50679
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-50679/48907
|
|
A new Pull Request was created by @ariostas for master. It involves the following packages:
The following packages do not have a category, yet: RecoTracker/LSTGeometry @Martin-Grunewald, @Moanwar, @cmsbuild, @jfernan2, @mandrenguyen, @mmusich, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild, please test |
|
-1 Failed Tests: UnitTests HLTP2Timing Failed Unit TestsI found 1 errors in the following unit tests: ---> test test-das-selected-lumis had ERRORS Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 17 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
|
Is ~190 MB increase in memory usage expected? |
That seems a bit high, but it's likely. I'll double-check. Either way, it is only temporarily. Most of it is freed once the maps are constructed. |
According to the monitoring the peak memory usage would increase by ~190 MB, and thus freeing it afterwards doesn't help much if the job was killed because of going over the limit. |
|
test parameters:
|
|
@cmsbuild, please test Maybe one round of profiling tests would be worth it. |
|
@fwyzard said
it could happen if there is no other work that can be done by all the Events (i.e. insufficient concurrency within an Event at that point in the schedule) |
|
@fwyzard yeah, it's an |
|
@ariostas ah, thanks for the clarification. Is the spike is caused by the |
|
@fwyzard the spike is caused by the |
|
OK, thanks for confirming it. |
|
Milestone for this pull request has been moved to CMSSW_20_0_X. Please open a backport if it should also go in to CMSSW_17_0_X. |
|
(I assume this PR does not need a backport to 17_0_X (Run 3 legacy)) |
|
Milestone for this pull request has been moved to CMSSW_20_1_X. Please open a backport if it should also go in to CMSSW_20_0_X. |
|
Pull request #50679 was updated. @Martin-Grunewald, @Moanwar, @cmsbuild, @jfernan2, @mandrenguyen, @mmusich, @srimanob can you please check and sign again. |
|
I added a new commit with some AI-assisted optimizations that bring down the time of this producer under 1 second. Hopefully this is now quick enough that it will solve the issue with kernel scheduling. |
|
please test |
|
-1 Failed Tests: HLTP2Timing Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 19 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
I think tests were sent prematurely here (tagging @jfernan2), let's wait to have a working IB including #51216 to have the baseline measurement and then repeat. |
|
@cmsbuild, please test
|
|
+1 Size: This PR adds an extra 16KB to repository HLT P2 Timing: chart Comparison SummarySummary:
Max Memory Comparisons exceeding threshold@cms-sw/core-l2 , I found 19 workflow step(s) with memory usage exceeding the error threshold: Expand to see workflows ...
|
indeed the resource usage looks better in the last round of tests: link.
overall change in timing is negligible link 
|
|
+hlt
|
|
Just for the record, this PR seems to cause tiny widespread changes in HLT tracking and related objects link for a more comprehensive summary. |
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @ftenchini, @sextonkennedy, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
small changes were seen in the LST integration tests. The old bin/text file for the LST geometry was derived from the tkLayout csv file; at least some rounding effects would contribute from this. |
|
Yeah, the small differences are expected, as Slava mentioned. There were a couple of minor changes with respect to the old maps. The old computation artificially made the modules a bit larger than they actually were and also the center of the modules was displaced a little bit by mistake. So most of the diffences are due to those changes, and the rest are just from floating point errors. I'm currently on vacation typing this on my phone, but I can give a more detailed changelog when I get back, if you prefer. |
|
+1 |
10 participants
This PR adds a new
RecoTracker/LSTGeometrypackage containing the module map computation used by the LST algorithm. Currently, the maps are pre-computed by the code in https://github.com/SegmentLinking/LSTGeometry and they are stored in https://github.com/cms-data/RecoTracker-LSTCore. This PR allows for the on-the-fly computation of these maps via an ESProducer, ensuring that they stay consistent with the tracker geometry being used.This is the last major task in #46746.
c.c. @slava77