Configuration arguments for version and packing schema of covariance in DeepJetTag producer #43917
Conversation
…s for hlt_features constructed from reco::PFCandidates
|
cms-bot internal usage |
mbluj
changed the title
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43917/38768
|
|
A new Pull Request was created by @mbluj for master. It involves the following packages:
@jfernan2, @cmsbuild, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -826,7 +832,7 @@ void DeepBoostedJetTagInfoProducer::fillParticleFeatures(DeepBoostedJetFeatures | |||
| pv_ass.key()); | |||
| candidate.setAssociationQuality(pat::PackedCandidate::PVAssociationQuality( | |||
| btagbtvdeep::vtx_ass_from_pfcand(*reco_cand, pv_ass_quality, pv_ass))); | |||
| candidate.setCovarianceVersion(0); | |||
| candidate.setCovarianceVersion(covarianceVersion_); | |||
There was a problem hiding this comment.
mmmh, this makes me realize that this was hard-coded for phase-0...
Shouldn't this be dealt with via era modifier as elsewhere in cmssw?
cmssw/PhysicsTools/PatAlgos/python/slimming/packedPFCandidates_cfi.py
Lines 31 to 32 in 4a6d7e8
cmssw/PhysicsTools/PatAlgos/python/slimming/lostTracks_cfi.py
Lines 29 to 30 in 4a6d7e8
EDIT: in any case that won't solve configuring at HLT which is independent of eras.
There was a problem hiding this comment.
Exactly, it is HLT configuration which is not era dependent in the same sense as offline. I do not know if it was intention of developers of the module to use phase0 pixel setting - it is question to BTV people.
This development is to help dealing with those settings using configuration parameters (maybe it can be checked before 2024 data-taking if usage of 1 is fine) without need to modify hard-coded vales.
There was a problem hiding this comment.
@cms-sw/btv-pog-l2 can you clarify about the intention of the hardcoded phase-0 packing scheme for HLT?
There was a problem hiding this comment.
Apparently, this was a mistake by the original author. He mistook the default value in the producer of 0 as the default. It clearly should be handled as discussed above. I did a study on 70k jets, and the current generation of taggers is unaffected by this choice, we can change it.
There was a problem hiding this comment.
hi @SWuchterl
Apparently, this was a mistake by the original author. He mistook the default value in the producer of 0 as the default. It clearly should be handled as discussed above
thanks for cross-checking! Once this PR is accepted and the new configuration is parsed, can you ask your HLT L3s to open a JIRA ticket to CMSHLT in order to change the configuration in the HLT menu for 2024 (assuming you want to move to the packing scheme for phase-1) ?
|
please test |
|
@mbluj, given the feedback at #43917 (comment) I think we'll need a backport PR to CMSSW_14_0_X. |
|
type btv |
OK, I will do it quickly as it will be a verbatim backport. |
|
are tests stuck here ? @smuzaffar |
|
@mmusich , no idea why bot did not start the test job. I see that https://cmssdt.cern.ch/jenkins/job/ib-schedule-pr-tests/69914/console had properly triggered https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests but now I do not find any ref to this PR. Anyway, I have restarted the test job |
|
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
|
urgent |
|
@cmsbuild, please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1ed9b2/37436/summary.html Comparison SummarySummary:
|
|
+1 |
cmsbuild
added
reconstruction-approved
fully-signed
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. @sextonkennedy, @rappoccio, @antoniovilela (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This PR adds configuration arguments for version and packing schema of covariance to
DeepBoostedJetTagInfoProducermodule.Those are used in by the module when construction of
hlt_featuresis enabled and when the features are constructed fromreco::PFCandidates, i.e. when the module is run at HLT. In the current code those are hard-coded which causes issues when the HLT RECO and miniAOD steps are run together in one workflow as discussed in #43861 issue.The implementation is just a technical change and do not change behavior of the module as default values of newly added configuration arguments are equal to values which were hard-coded.
PR validation:
Verified that newly added parameters do not change previously used values.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
Foreseen backport to 14_0_X release series that will be used for 2024 operations.