-
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
remove redundant customizations in HcalNano #45561
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45561/41044 |
A new Pull Request was created by @jhakala for master. It involves the following packages:
@cmsbuild, @ftorrresd, @hqucms, @vlimant can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
enable nano |
please test |
+1 Size: This PR adds an extra 20KB to repository 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:
|
@@ -35,25 +35,7 @@ def customiseHcalCalib(process): | |||
process.raw2digi_step = cms.Path(process.hcalCalibDigiSequence, process.RawToDigiTask) | |||
|
|||
# Insert the HLT filter at start of user path and nanoaod endpath | |||
process.user_step.insert(0, process.hcalCalibHLTFilter) | |||
process.nanoAOD_step.insert(0, process.hcalCalibHLTFilter) |
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.
a safer way, IMO, would be to list of path of the process and insert the filter in first position, instead of calling explicitly nanoAOD_step
and NANOAODoutput_step
without checking their existence in the process
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 seems to work when I replace that line with:
process.paths.values()[0].insert(0, process.hcalCalibHLTFilter)
Does that seem reasonable to you? Or is there a more preferred way to do this? (I don't see this trick anywhere else in CMSSW with an LXR search...)
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.
something like
for path in self.process.paths:
path.insert(0,process.hcalCalibHLTFilter)
I think. and verify that it does prepend it to the output path too at the next line
@vlimant I added a commit that (I think) takes care of your suggestion. The output path was an EndPath, so I added the filter to the beginning of everything in |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45561/41082 |
Pull request #45561 was updated. @cmsbuild, @ftorrresd, @hqucms, @vlimant can you please check and sign again. |
please test |
+1 Size: This PR adds an extra 16KB to repository Comparison SummarySummary:
NANO Comparison SummarySummary:
Nano size comparison Summary:
|
+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, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This is a code cleanup/simplification for HcalNano, plus a one-line change in a related runTheMatrix workflow. The change in
PhysicsTools/NanoAOD/python/autoNANO.py
is because of a removal of a duplicate function -- the change is to point it to the preserved version of the function.PR validation:
I ran the related workflows:
2500.313_hcalDPGCalibNANO130Xrun3
and2500.312_hcalDPGNANO130Xrun3
(which until recently were numbered 1060.1 and 1060.2). They looked fine.No backport is needed.