-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Create new gtStage2Digis module for Run-3 scouting #42484
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
Create new gtStage2Digis module for Run-3 scouting #42484
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42484/36490
|
|
A new Pull Request was created by @alintulu (Adelina Lintuluoto) for master. It involves the following packages:
@vlimant, @bbilin, @clacaputo, @cmsbuild, @AdrianoDee, @srimanob, @simonepigazzini, @sunilUIET, @mandrenguyen can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
perrotta
left a comment
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.
@alintulu this PR is identical to previous #40438 but the fixes applied to PhysicsTools/NanoAOD/python/custom_run3scouting_cff.py
Making a new PR will loose all the discussions you had in that thread (181 entries since January!), or at least it will make them not so easy to retrieve.
I think that for documentation purposes it would be better to modify the original PR and finally merge it, together with its whole story.
Regardless of that, the updates implemented here look correct, at the first sight. Let test them here; but as I said it would be preferable to update and finally merge the original PR.
| '--datatier':'NANOAOD', | ||
| '--eventcontent':'NANOAOD'}]) | ||
|
|
||
| steps['NANO_mcRun3ScoutingPF13.X']=merge([{'-s':'NANO:PhysicsTools/NanoAOD/custom_run3scouting_cff.nanoSequenceMC', |
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.
Copying here the comment of @vlimant in the original PR (that could even had been addressed here, since you are making a new PR):
I believe that the .nanoSequenceMC is not necessary, since --mc will have cmsDriver pick that one up automatically. We'll deal with this at a later stage during the NANO:@Scouting integration
|
enable nano |
|
please test |
@perrotta Sorry I misunderstood you, I thought you preferred a new PR. I can of course add the changes to the original, however in order to avoid recollecting all the signatures on the last PR, maybe the following could be an option?
|
|
-1 Failed Tests: RelVals-INPUT RelVals-NANO RelVals-INPUT
RelVals-NANO
Comparison SummarySummary:
|
|
Looking at the log, I think the test failed because of the missing PR to RecoBTag-Combined (cms-data/RecoBTag-Combined#49). It was added to the other PR by #40438 (comment). |
|
test parameters: |
|
please test |
|
@alintulu #40438 cannot be merged as such, because it is bugged. It must be merged together with this one, which implies recollecting all signatures once again. Either you make a new PR with only the updated PhysicsTools/NanoAOD/python/custom_run3scouting_cff.py, which will likely require only the xpog signature, and the two PRs can therefore get tested and merged together. Or we merge the overall PR with the bug fixed, as it is this one now. If we decide for the complete PR, better to stick on the old one, adding the fix to it, so that the whole set of comments can be saved for reference |
Ah I see, I didn't realise I could make a PR with just one file change which could then be tested together with the old one. Thanks! I don't have a preference, but maybe that would be the easiest? If you agree, I'll go ahead and make the PR with only the PhysicsTools/NanoAOD/python/custom_run3scouting_cff.py change. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0da083/34124/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 SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0da083/34150/summary.html Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
|
@missirol @Martin-Grunewald do you have any clue about the configuration error reported in the PR description? |
|
Looks like the message comes from |
|
We would like to close this PR and have you apply the changes in the original PR #40438, please. |
|
Replaced by #40438 |
PR description:
This PR is to address the concern brought up in #40438 (comment). It creates a new module for gtStage2Digis, as well as every module needed to update the configuration given the initial change. The new modules are named after the original, with a suffix of
Scouting(e.g.gtStage2Digis->gtStage2DigisScouting), however please let me know if there is a more appropriate naming strategy.The only difference between this and PR #40438, are the changes described above made to PhysicsTools/NanoAOD/python/custom_run3scouting_cff.py.
PR validation:
The PR passed the runTheMatrix tests affiliated with #40438. The
Cannot unpack: no FEDRawDataCollection foundmessages related togmtStage2DigisandcaloStage2Digisare gone, howeverNo HLT information producedis still present (an example displayed below).