cmssw
cmssw copied to clipboard
cms-sw
change Selector from bit pattern to bit number for the pattern
PR description:
this is a change in the muon Selector enumerator format to avoid going over numerical range I understand this was already discussed and agreed upon, but I am opened to suggestions.
PR validation:
fixes : #38899 nicely
A new Pull Request was created by @vlimant (vlimant) for CMSSW_12_4_X.
It involves the following packages:
- DataFormats/MuonReco (reconstruction)
@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks. @sscruz, @Fedespring, @battibass, @abbiendi, @rovere, @HuguesBrun, @jhgoh, @CeliaFernandez, @trocino, @amagitte, @cericeci this is something you requested to watch as well. @perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.
cms-bot commands are listed here
- Backported from #38948
New categories assigned: xpog
@mariadalfonso,@gouskos,@swertz,@vlimant you have been requested to review this Pull request/Issue and eventually sign? Thanks
from https://github.com/cms-sw/cmssw/issues/38899#issuecomment-1200043575
The hard way to fix : https://github.com/cms-sw/cmssw/compare/CMSSW_12_4_X...vlimant:cmssw:fw_change_for_nano_muon_masks letting the parser always use long for enumerators.
Shouldn't the parser still be fixed to allow support of long ints?
+1
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6ccbfb/26607/summary.html
COMMIT: cdf932271cc33694bb444b784333e886129880d2
CMSSW: CMSSW_12_4_X_2022-08-02-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38943/26607/install.sh to create a dev area with all the needed externals and cmssw changes.
Comparison Summary
Summary:
- No significant changes to the logs found
- Reco comparison results: 270 differences found in the comparisons
- DQMHistoTests: Total files compared: 50
- DQMHistoTests: Total histograms compared: 3676198
- DQMHistoTests: Total failures: 1072
- DQMHistoTests: Total nulls: 1
- DQMHistoTests: Total successes: 3675103
- DQMHistoTests: Total skipped: 22
- DQMHistoTests: Total Missing objects: 0
- DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
- DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
- Checked 208 log files, 45 edm output root files, 50 DQM output files
- TriggerResults: no differences found
@sscruz we have a lot of changes in the mini can you look at the results here https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6ccbfb/26607/summary.html
see for example from all_OldVSNew_TTbar14TeV2026D88PUwPRMXwf39634p999
similarly from Run3, see: all_OldVSNew_TTbar14TeVPU2021wf11834p0
For nano we have changes in fatJetTable, jetTables, simpleCleanerTable, fsrTable all related to the muon selection and of course in muonTable and mcTable.
thanks for the pointer. Some additional changes to pat::Muon were needed, I've done them here https://github.com/sscruz/cmssw/commit/e041345e2563d5a39ac8752ed207475c2b024195. @vlimant could you cherry-pick this commit or let me know which IB or release was used so I can make a PR to you branch, whatever works best for you.
Besides the changes I've done, I've also realized that here https://github.com/cms-sw/cmssw/blob/master/DQM/GEM/python/gemEfficiencyAnalyzer_cff.py#L20 they are using the passed(8) method, precisely to by-pass the problem we are trying to fix in this PR. I think we should also modify that line if it's fine with the DQM developers
I've done them here sscruz@e041345
is it obvious that other developers do not use the same pattern in the private code? It still seems to me that fixing the string selector is a more appropriate way to go.
I've done them here sscruz@e041345
is it obvious that other developers do not use the same pattern in the private code? It still seems to me that fixing the string selector is a more appropriate way to go.
the changes in sscruz@e041345 do not have to do with the fact that the passsed(8)-pattern would be broken.
Regarding what is done in the user code, I personally think that's not the way one should access the selector information, however I don't have any strong preference for any of the two options...
Pull request #38943 was updated. @gouskos, @swertz, @vlimant, @clacaputo, @cmsbuild, @jpata, @mariadalfonso can you please check and sign again.
@sscruz cherrypick done. I'll try to update #38948. looks cumbersome to do cross-cherrypicking thus far
Pull request #38943 was updated. @gouskos, @swertz, @vlimant, @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @clacaputo, @jpata, @mariadalfonso, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.
+1
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6ccbfb/26683/summary.html
COMMIT: 6e48fba34afc0ac131eb16acc1ef9610c30b55f8
CMSSW: CMSSW_12_4_X_2022-08-07-0000/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38943/26683/install.sh to create a dev area with all the needed externals and cmssw changes.
Comparison Summary
Summary:
- No significant changes to the logs found
- Reco comparison results: 151 differences found in the comparisons
- DQMHistoTests: Total files compared: 50
- DQMHistoTests: Total histograms compared: 3677242
- DQMHistoTests: Total failures: 1109
- DQMHistoTests: Total nulls: 0
- DQMHistoTests: Total successes: 3676111
- DQMHistoTests: Total skipped: 22
- DQMHistoTests: Total Missing objects: 0
- DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
- Checked 208 log files, 45 edm output root files, 50 DQM output files
- TriggerResults: no differences found
Comparison Summary
Summary:
- No significant changes to the logs found
- Reco comparison results: 151 differences found in the comparisons
These are in the nano
| WF # | Differences |
|---|---|
| all_OldVSNew_TTbar13nanoEDM106Xv1in2017wf1325p81 | 75 |
| all_OldVSNew_RunJetHT2018CnanoEDM106Xv2wf136p8523 | 72 |
mainly muon and related variables
@sscruz can you have a look at the results ? https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_5_X_2022-08-07-0000+8c1f44/52010/validateJR.html
It look sensible to me and I'm ready to sign.
Some plot
I will sign this after the master version, which is currently not identical to this PR.
+reconstruction
- backport of https://github.com/cms-sw/cmssw/pull/38948 (equivalent up to formatting around
<<) - fixes https://github.com/cms-sw/cmssw/issues/38899
- changes to NANO and nothing else
This pull request is fully signed and it will be integrated in one of the next CMSSW_12_4_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_5_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)
(Besides a few spaces fixed by the code-checks, this is a verbatim backport of https://github.com/cms-sw/cmssw/pull/38943)
