Speedups to HGCal clustering #16997
Speedups to HGCal clustering #16997
Conversation
|
A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_9_0_X. It involves the following packages: RecoLocalCalo/HGCalRecAlgos @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@cmsbuild please abort better start with a new IB present |
|
Jenkins tests are aborted. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
this is still too early |
|
please abort
…On Tue, Dec 13, 2016 at 11:06 AM, Slava Krutelyov ***@***.***> wrote:
this is still too early
there is no working baseline with multicluster PR included.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16997 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABBMOejPznvm4sD_zGOXc2Kk1n24G3Amks5rHtCigaJpZM4LLMOF>
.
|
|
Jenkins tests are aborted. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 6e45d59 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test runtestRecoLocalCaloHGCalRecProducers had ERRORS |
| @@ -83,14 +85,16 @@ math::XYZPoint ClusterTools::getMultiClusterPosition(const reco::HGCalMultiClust | |||
| for( const auto& ptr : clu.clusters() ) { | |||
| const double x = ptr->x(); | |||
| const double y = ptr->y(); | |||
| const float point_r = std::sqrt(x*x + y*y); | |||
| const double point_r2 = (x*x + y*y); | |||
There was a problem hiding this comment.
is double-precision well justifiable for the math here?
There was a problem hiding this comment.
everything else is already in double, repeated conversion from double to float costs time.
There was a problem hiding this comment.
and this is in a tight loop
There was a problem hiding this comment.
the reason for x,y,z and energy to be in double is debatable on the storage side.
there is nothing else inside this loop that requires double.
OK for now
| return std::sqrt(distance2(pt1,pt2)); | ||
| } | ||
|
|
||
| double HGCalImagingAlgo::distance2(const Hexel &pt1, const Hexel &pt2){ |
|
@cmsbuild please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-approved
labels
|
The tests are being triggered in jenkins. |
|
Pull request #16997 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again. |
|
Comparison job queued. |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
|
+1 |
Addressing comments made at the end of: #16909
This PR significantly decreases the number of calls to sqrt() in tight loops and uses vdt where useful in the HGCal layer and multiclusters.