Updated MLPF producer with ONNX #36841
Conversation
|
test parameters:
|
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36841/28032
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36841/28033
|
|
adding @cms-sw/pf-l2 @laurenhay here, just to make sure everyone is informed. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bafd5c/22230/summary.html The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see more details here: Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
+1 |
|
+reconstruction
|
|
+Upgrade From the code related to Upgrade, updating the MLPF workflow to allow QCD sample is fine. |
|
+pdmv |
cmsbuild
added
fully-signed
pdmv-approved
and removed
pending-signatures
pdmv-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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
| tensorflow::Tensor input(tensorflow::DT_FLOAT, shape); | ||
| input.flat<float>().setZero(); | ||
| #ifdef MLPF_DEBUG | ||
| std::cout << "tensor_size=" << tensor_size << std::endl; |
There was a problem hiding this comment.
Is this needed, given the previous assert's?
If really needed to debug, then maybe better to have this cout before L55
There was a problem hiding this comment.
true, it would perhaps be more informative (and less lines) to print out before the assert while debugging. can we address this in a follow-up version, or would you prefer a resigning of this PR?
There was a problem hiding this comment.
Ok, do not request 4+1 signatures only to move one comment line: let postpone it to a follow-up PR
| cand.setPdgId(pred_pid); | ||
| cand.setCharge(charge); | ||
| reco::PFCandidate::ParticleType particleType(reco::PFCandidate::X); | ||
| if (pred_pid == 211) |
There was a problem hiding this comment.
It looks like this pred_pid is unsigned (here and everywhere else in this code), i.e. it does not consider the charge: can you confirm, just for check?
There was a problem hiding this comment.
Yes, by construction, the underlying model reconstructs the absolute value of the PID separately from the charge.
So far, we didn't present detailed results about the charge prediction (so //cand.setCharge(charge); downstream), but it should not be a major problem, as should be driven by the track information.
|
+1
|
PR description:
Following the presentation at the PPD general meeting, at ACAT2021, and as outlined in the PPD workshop, we are updating the MLPF integration in CMSSW to facilitate further development and scrutiny.
Note that MLPF is off by default and thus no changes are expected in any of the standard workflows.
This PR mainly updates the ML model and switches the inference to ONNX from tensorflow. MLPF-specific event content is removed, as now MLPF produces PFCandidates instead of (rather than in parallel to) PFAlgo, when enabled.
Here are the igprof results:
PR validation:
For physics validation, please see the slides linked above. The integration can be tested in the workflows 11843.13 and 11834.13.