Fix features order #29799
Fix features order #29799
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29799/15284
|
|
A new Pull Request was created by @rovere (Marco Rovere) for master. It involves the following packages: RecoHGCal/TICL @perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
Comparison differences are limited to Phase 2 workflows, as expected. @rovere I notice a few potentially surprising changes. Please confirm if these are expected:
|
|
Ciao @kpedro88 thanks for checking. The problem is that the comparison, in this case, does not make too much sense, since the ML inference was wrong before and, as a consequence, all the results produced in terms of PID and energy regression were unreliable. |
| @@ -333,9 +333,9 @@ void PatternRecognitionbyCA::energyRegressionAndID(const std::vector<reco::CaloC | |||
| float *features = &input.tensor<float, 4>()(i, j, seenClusters[j], 0); | |||
|
|
|||
| // fill features | |||
There was a problem hiding this comment.
This "interface" is extremely error prone (as this bug just demonstrated).
Whoever approved this code for CMSSW clearly was not doing their job.
There was a problem hiding this comment.
Looks like the original sin is #27917.
What are the plans to fix the use of anonymous, unordered list for passing parameters to tensorflow or other ML engines ?
There was a problem hiding this comment.
@fwyzard this is a good point. Maybe the framework should add a data structure like edm::featureMap that enforces a schema and has interfaces to output in various ML formats.
(Though, this would only prevent one of the two problems addressed by this PR. No framework feature can specify whether or not energy should be divided by vertex multiplicity...)
There was a problem hiding this comment.
(Though, this would only prevent one of the two problems addressed by this PR. No framework feature can specify whether or not energy should be divided by vertex multiplicity...)
:-)
There was a problem hiding this comment.
Opened #29818 to follow this
|
@rovere I take the point that the "old" results are just wrong. However, can you remark if the "new" results seem correct? |
|
@kpedro88 I can confirm that using single-pion and single-electron guns into HGCAL the network is able to assign the proper ID to the reconstructed |
|
+upgrade |
|
+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 master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
In recent meetings at the Upgrade HLT TDR, strange distribution of PFCandidates produced from TICL have been reported: JetMET BTV.
After some debugging, this has been traced back to the way in which we pass features to ML model for PID and energy regression. This PR reshuffle them in the correct order. Local tests show no more dependency on phi.
PR validation:
Besides checking that the strange phi behaviour is gone:
runTheMatrix.py -l limited