Skip to content
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

HBHE: arrival time [ backport 12_4 ] #39365

Merged
merged 13 commits into from Sep 18, 2022

Conversation

mariadalfonso
Copy link
Contributor

backport of #39227

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 10, 2022

A new Pull Request was created by @mariadalfonso for CMSSW_12_4_X.

It involves the following packages:

  • DataFormats/HcalRecHit (reconstruction)
  • RecoLocalCalo/HcalRecAlgos (reconstruction)
  • RecoLocalCalo/HcalRecProducers (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@youyingli, @bsunanda, @rchatter, @rovere, @argiro, @apsallid, @thomreis, @simonepigazzini, @abdoulline this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-bf3186/27466/summary.html
COMMIT: 42fb0c1
CMSSW: CMSSW_12_4_X_2022-09-11-0000/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39365/27466/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: 675 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3677396
  • DQMHistoTests: Total failures: 1035
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3676338
  • 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

@mandrenguyen
Copy link
Contributor

urgent
As discussed at Joint ops anything intended for the resumption of collision data should be marked as urgent.

@rovere
Copy link
Contributor

rovere commented Sep 15, 2022

Ping to the reconstruction convener. This PR will be mandatory for data-taking that could happen as early as Tuesday next week.

@mandrenguyen
Copy link
Contributor

Hi @rovere We are waiting for MET experts to confirm that they're ok with changes that modify the beam halo filters before approving.

@rovere
Copy link
Contributor

rovere commented Sep 15, 2022

Thanks @mandrenguyen for the update.

@rovere
Copy link
Contributor

rovere commented Sep 16, 2022

Dear all, LHC is going very well and the plan is to have stable beams as early as next week.
We need to finalize the release for the coming data-taking by today.
Do you think you'll receive feedback from MET experts in time?
The original PR against master was opened 18 days ago.

@mandrenguyen
Copy link
Contributor

mandrenguyen commented Sep 16, 2022

Dear @rovere We have a presentation on this PR scheduled for the RECO meeting, which starts at 3:30PM CEST today. I hope we can clarify the matter there.

@rovere
Copy link
Contributor

rovere commented Sep 16, 2022

Ciao @mandrenguyen thanks a lot.
We'll see how it goes.

@mandrenguyen
Copy link
Contributor

+1
I'm going and signing as I cannot see how an improved HCAL timing estimate could be problematic. Nevertheless, it should still be considered whether it's acceptable to put this change into production for the prompt reco about to start in a few days. For that I believe we should get the ok from JetMET before proceeding.

@cmsbuild
Copy link
Contributor

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_6_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1 I'm going and signing as I cannot see how an improved HCAL timing estimate could be problematic. Nevertheless, it should still be considered whether it's acceptable to put this change into production for the prompt reco about to start in a few days. For that I believe we should get the ok from JetMET before proceeding.

We have to close now the release for the resuming data taking. It is unfortunate that @cms-sw/jetmet-pog-l2 did not provide yet an evaluation of the possible effect of this change into the halo filters, but now a decision has to be taken: relying on the opinions of the main HCAL experts around, and based on the reco signature, we conclude that this PR should be considered for the remaining part of the 2022 data taking, and for the related MC production, and therefore we are merging this PR also in 12_4_X. We still have a few hours ahead to revert it in case of any late motivated objection from jetmet.

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 43ce9a1 into cms-sw:CMSSW_12_4_X Sep 18, 2022
@alkaloge
Copy link
Contributor

alkaloge commented Sep 18, 2022

The MET scanners team has been already notified, but naively I would say that we will only know once we get data with the new PR, and then the METscanners team can have a look at the impact.

@mandrenguyen
Copy link
Contributor

@alkaloge For info, JetMET was pinged in the PR thread 7 days ago, very shortly after the final comparisons became available. Before that the PR itself was under evolution, so I could not evaluate what was impacted by the PR.

@alkaloge
Copy link
Contributor

@alkaloge For info, JetMET was pinged in the PR thread 7 days ago, very shortly after the final comparisons became available. Before that the PR itself was under evolution, so I could not evaluate what was impacted by the PR.

agreed, it is just that it is hard to evaluate any PR that can affect data, before collecting data and have a look on it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants