-
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
[PF] Add fillDescription to RecoParticleFlow producers #45212
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45212/40564
|
A new Pull Request was created by @stahlleiton 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 |
type pf |
@cmsbuild please test |
@cms-sw/hlt-l2 FYI |
quoting from the originating issue:
Was the removal of |
was this tested with
|
I think the problem is here:
which makes me also doubt about:
|
-1 Failed Tests: RelVals RelVals-INPUT AddOn RelVals
RelVals-INPUT
AddOn Tests
|
desc.setUnknown(); | ||
descriptions.addDefault(desc); | ||
edm::ParameterSetDescription psd; | ||
psd.setUnknown(); |
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.
Using setUknown really defeats the purpose of the fillDescriptions.
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.
The issue I had in this particular class was that there were so many variations of this parameter set (in different python configuration files scattered across CMSSW), that failed to find a super set that cover all (so end up declaring it as unknown). Same with PFClusterProducer. I would have to think if there is a way to avoid having a long list of optional parameters.
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.
The unknown parameter set descriptions are removed in 2da72f0
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.
@stahlleiton just a word of caution about the (many) addOptional
added in 2da72f0. Currently the ConfDB
parsing skips any cms.optional
in the resulting cfi files, so if at some point any of these need to be configured at HLT it won't be possible (unless a default is provided in the C++ source code).
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.
I made them optional because there were parameter sets with different content used in different parts of CMSSW. So I only used add for those parameters that were common among all parameter sets.
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.
Removed the optional condition from all the parameters I added in the fillDescription in c85415b
RecoParticleFlow/PFClusterProducer/plugins/PFClusterProducer.cc
Outdated
Show resolved
Hide resolved
Not yet, and in this PR I only added the fill descriptions and was planning to fix the "exist" and "existAs" in another PR. |
I tested it with pp and UPC relvals, forgot to also include HI relvals. Will fix and test again |
Pull request #45212 was updated. @mandrenguyen, @Martin-Grunewald, @cmsbuild, @jfernan2, @mmusich can you please check and sign again. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45212/40644
|
Pull request #45212 was updated. @mandrenguyen, @cmsbuild, @jfernan2 can you please check and sign again. |
@cmsbuild, please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f69da3/39965/summary.html Comparison SummarySummary:
|
Are there further comments on this PR? |
+1 |
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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR add the fillDescription to the producers in the RecoParticleFlow directory that were missing it. In addition, it also replaces the getByLabel with getByToken. This PR is related to the issue #43169 .
PR validation:
Tested that it compiles and ran 5 relvals locally.
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:
@swagata87