CLUE3D Bug Fixes #36857
CLUE3D Bug Fixes #36857
Conversation
Fix bug related to unsigned int subtraction. Also, add a customize python function to allow to save in output only the data-products that are needed to run the HGCAL (local) reconstruction from. Finally, increase the search window in eta to 2, for all eta values.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36857/28098
|
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages:
@clacaputo, @cmsbuild, @AdrianoDee, @srimanob, @slava77, @jpata can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b70d6/22135/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
| int etaWindow = onSameLayer ? 2 : 1; | ||
| int phiWindow = onSameLayer ? 2 : 1; | ||
| const int etaWindow = 2; | ||
| const int phiWindow = 2; |
There was a problem hiding this comment.
Just for my knowledge, I most probably have missed some passages: why this was restricted to 1 for layerClusters on different layers?
There was a problem hiding this comment.
Ciao @AdrianoDee, the original motivation for having 2 different cuts, within and outside of the layer, is related to the fact that we are clustering in 3D LayerClusters. Therefore there should be no need to further cluster them within every single layer or, if you want, you would need to search a little farther away to find some other clusters to collect on the same layer. The move to 2 was taken from the CA experience: we notice that, especially at high eta values, a single bin in eta,phi can be quite restrictive. Hence to move to just use one value, 2, to cover them all. When we reach the point of including CLUE3D in the reconstruction, we will need, possibly, some further tuning.
| @@ -718,7 +719,7 @@ void PatternRecognitionbyCLUE3D<TILES>::fillPSetDescription(edm::ParameterSetDes | |||
| iDesc.add<bool>("densityOnSameLayer", false); | |||
| iDesc.add<double>("criticalEtaPhiDistance", 0.035); | |||
| iDesc.add<double>("outlierMultiplier", 2); | |||
| iDesc.add<int>("minNumLayerCluster", 5)->setComment("Not Inclusive"); | |||
| iDesc.add<int>("minNumLayerCluster", 2)->setComment("Not Inclusive"); | |||
There was a problem hiding this comment.
(As above, I have surely missed something). Why is this going from 5 to 2?
There was a problem hiding this comment.
Ciao @AdrianoDee in this case there's no real reason. The idea, in this case, is that the selection will be done on top of this and will likely use more advanced ML techniques. For this reason, it could be better to allow more input to possibly gather a little more in output. I hope this is clear.
There was a problem hiding this comment.
I see. Thanks @rovere for all the details (here and above).
|
+Upgrade |
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-approved
upgrade-approved
code-checks-approved
labels
|
test parameters:
|
|
@cmsbuild please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36857/28140
|
|
Pull request #36857 was updated. @jpata, @AdrianoDee, @srimanob, @clacaputo, @slava77 can you please check and sign again. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b70d6/22201/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
+Upgrade |
|
Hi @rovere, I've tested |
|
Thanks @clacaputo for the additional checks. The differences will not be dramatic and will impact mainly the tracksters in the very forward region (eta 2.5 or higher, more or less). The samples you have used generate particles at eta |
|
@rovere, thanks a lot for the clarification |
|
+reconstruction
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
reconstruction-pending
pending-signatures
labels
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
| auto lastLayerPerSide = static_cast<unsigned int>(rhtools_.lastLayer(false)) - 1; | ||
| unsigned int minLayer = 0; | ||
| unsigned int maxLayer = 2 * lastLayerPerSide - 1; | ||
| auto lastLayerPerSide = static_cast<int>(rhtools_.lastLayer(false)); |
There was a problem hiding this comment.
This is not the same as previous lastLayerPerSide, and maxLayer as well as a consequence: please confirm that this is wanted
There was a problem hiding this comment.
Ciao @perrotta that's correct, the -1 was a bug as well.
|
+1 |
PR description:
Fix bug related to unsigned int subtraction. Also, add a customize
python function to allow to save in output only the data-products that
are needed to run the HGCAL (local) reconstruction from. Finally,
increase the search window in eta to 2, for all eta values.
PR validation:
Tested using
runTheMatrixworkflows withclue3Denabled.