-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding offlineVerticesWithBS to miniAOD #33778
Adding offlineVerticesWithBS to miniAOD #33778
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33778/22745
|
A new Pull Request was created by @AdrianoDee for master. It involves the following packages: PhysicsTools/PatAlgos @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like use cases for most of the introduced producers are missing.
shouldn't there be at least a keep statement for the output event format?
|
||
run2_miniAOD_pp_on_AA_103X.toModify(primaryVertexWithBSAssociation,particles = "cleanedParticleFlow") | ||
primaryVertexWithBSAssociationCleaned = primaryVertexWithBSAssociation.clone(particles = "cleanedParticleFlow:removed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sincerely I placed it for consistency, for me we may erase it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandrenguyen @abaty could you please give your advice about the possible need of these offlineVerticesWithBS
for HI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have offlinePrimaryVerticesWithBS in HI miniAOD as well. I checked the increase in size on 100 events of wf 140.5611 and it looks negligible, around 0.1%. PU is so low in PbPb that multiple vertices are more likely to come from vertex splitting. So perhaps this is not unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have offlinePrimaryVerticesWithBS in HI miniAOD as well. I checked the increase in size on 100 events of wf 140.5611 and it looks negligible, around 0.1%. PU is so low in PbPb that multiple vertices are more likely to come from vertex splitting. So perhaps this is not unexpected.
Great, thank you Matt,
Therefore, this part can remain as it is,
Indeed you are right, I added a keep to the MicroEventContent (for miniAOD) |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33778/22755
|
@@ -4,10 +4,17 @@ | |||
primaryVertexAssociation = sortedPFPrimaryVertices.clone( | |||
qualityForPrimary = cms.int32(2), | |||
produceSortedVertices = cms.bool(False), | |||
producePileUpCollection = cms.bool(False), | |||
producePileUpCollection = cms.bool(False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop type specifications for all parameters which already exist.
This is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.
primaryVertexAssociation = sortedPFPrimaryVertices.clone(
qualityForPrimary = 2,
produceSortedVertices = False,
producePileUpCollection = False,
produceNoPileUpCollection = False
)
produceNoPileUpCollection = cms.bool(False) | ||
) | ||
|
||
primaryVertexWithBSAssociation = primaryVertexAssociation.clone( | ||
vertices = cms.InputTag("offlinePrimaryVerticesWithBS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vertices = cms.InputTag("offlinePrimaryVerticesWithBS") | |
vertices = "offlinePrimaryVerticesWithBS" |
|
||
run2_miniAOD_pp_on_AA_103X.toModify(primaryVertexWithBSAssociation,particles = "cleanedParticleFlow") | ||
primaryVertexWithBSAssociationCleaned = primaryVertexWithBSAssociation.clone(particles = "cleanedParticleFlow:removed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandrenguyen @abaty could you please give your advice about the possible need of these offlineVerticesWithBS
for HI?
cms.Task(slimmingTask.copy(), packedCandidateMuonID, packedPFCandidateTrackChi2, lostTrackChi2, centralityBin, hiHFfilters)) | ||
from Configuration.ProcessModifiers.run2_miniAOD_pp_on_AA_103X_cff import run2_miniAOD_pp_on_AA_103X | ||
run2_miniAOD_pp_on_AA_103X.toReplaceWith(slimmingTask,cms.Task(primaryVertexAssociationCleaned,slimmingTask.copy())) | ||
run2_miniAOD_pp_on_AA_103X.toReplaceWith(slimmingTask,cms.Task(primaryVertexWithBSAssociationCleaned,slimmingTask.copy())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: insertion to be agreed with @mandrenguyen @abaty
assign xpog |
New categories assigned: xpog @fgolf,@mariadalfonso,@gouskos you have been requested to review this Pull request/Issue and eventually sign? Thanks |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33778/22796
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33778/22915
|
please test |
+1
|
@perrotta thanks |
+xpog miniAOD size increase in the description inline with the discussion of the budget in the xPOG |
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3bd2dd/15387/summary.html Comparison SummarySummary:
|
+1 |
PR description:
This PR adds a new collection of
offlineSlimmedPrimaryVerticesWithBS
to miniAOD format. This collection comes from AODofflinePrimaryVerticesWithBS
with the same slimming and association used forofflineSlimmedPrimaryVertices
.The relative size increase is checked on a a set of Run3 ttbar samples with multiple :
The size per event:
The v3 shown here is the version with
minPtForLowQualityTrackProperties=0.0
with #33777Further checks in the references below.
PR Validation and Further references
BPH Jamboree December 2020 here
xPOG Meeting February 2021 here
Further reference here
For a complete documentation, trk4bph meetings: 1, 2, 3 , 4, 5, 6