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

AlcaPCCEventProducer configuration to save per ROC data #46231

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

duff-ae
Copy link
Contributor

@duff-ae duff-ae commented Oct 3, 2024

A minor change to the "AlcaPCCEventProducer"

The proposed modification would enable external configuration of the producer. This would allow us to specify, via a menu, whether we want to save pixel data at per-module or per-ROC granularity.

This feature is particularly important for special fills where we have high-rate zero-bias streams for AlcaLumi. The proposed change could reduce event size by a factor of ~7 under these conditions compared to the current implementation. It is essential to have this feature for successful pp-Ref and HI vdM fills planned in the next weeks.

However, per-ROC information is critical for physics, as it helps us understand instabilities and non-linearities within individual modules, and we wish to continue recording this data. More details are given in the talk by B. Kronheim and C. Palmer.

The target release is 14_1

This PR is related to #29069 #44996

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

A new Pull Request was created by @duff-ae for master.

It involves the following packages:

  • Calibration/LumiAlCaRecoProducers (alca)

@atpathak, @cmsbuild, @consuegs, @perrotta can you please review it and eventually sign? Thanks.
@mmusich, @rsreds, @tocheng, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Oct 3, 2024

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented Oct 3, 2024

@cms-sw/hlt-l2 FYI

@@ -38,6 +38,7 @@ class AlcaPCCEventProducer : public edm::global::EDProducer<> {
private:
const edm::InputTag pixelClusterLabel_;
const std::string trigstring_; //specifies the trigger Rand or ZeroBias
bool savePerROCInfo_ = true; // save per ROC data (important for the special fills)
Copy link
Contributor

@Martin-Grunewald Martin-Grunewald Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const bool savePerROCInfo_; as for the other parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, added

@@ -101,6 +105,7 @@ void AlcaPCCEventProducer::fillDescriptions(edm::ConfigurationDescriptions& desc
edm::ParameterSetDescription evtParamDesc;
evtParamDesc.add<edm::InputTag>("pixelClusterLabel", edm::InputTag("siPixelClustersForLumi"));
evtParamDesc.addUntracked<std::string>("trigstring", "alcaPCCEvent");
evtParamDesc.addUntracked<bool>("savePerROCInfo", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own understanding: since this parameter affects heavily the physics output, should it be a tracked parameter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, makes perfect sense. I modified the code

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46231/42048

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

Pull request #46231 was updated. @atpathak, @cmsbuild, @consuegs, @perrotta can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

Pull request #46231 was updated. @atpathak, @cmsbuild, @consuegs, @perrotta can you please check and sign again.

@mmusich
Copy link
Contributor

mmusich commented Oct 3, 2024

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2024

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b56570/41945/summary.html
COMMIT: 8bb4def
CMSSW: CMSSW_14_2_X_2024-10-03-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/46231/41945/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: 99 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3331336
  • DQMHistoTests: Total failures: 2909
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3328407
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 193 log files, 163 edm output root files, 44 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2024

+alca

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 4, 2024

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. @mandrenguyen, @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@mmusich
Copy link
Contributor

mmusich commented Oct 4, 2024

@duff-ae

  • please update the PR title to be more descriptive of the changes
  • please open a backport to CMSSW_14_1_X

@duff-ae duff-ae changed the title Pcc per roc lumi AlcaPCCEventProducer configuration to save per ROC data Oct 4, 2024
@duff-ae
Copy link
Contributor Author

duff-ae commented Oct 4, 2024

@perrotta Dear Andrea, Could you please help us with the backport?

@mmusich
Copy link
Contributor

mmusich commented Oct 4, 2024

@duff-ae

Dear Andrea, Could you please help us with the backport

to speed-up integration, I did it. See #46249

@mandrenguyen
Copy link
Contributor

urgent

@cmsbuild cmsbuild added the urgent label Oct 4, 2024
@mmusich
Copy link
Contributor

mmusich commented Oct 4, 2024

@duff-ae FYI, making a backport is no different from making a PR to master, you just need to start from a different base branch.
Full recipe below:

cmsrel CMSSW_14_1_X_2024-10-03-2300
cd CMSSW_14_1_X_2024-10-03-2300/src/
cmsenv
git cms-addpkg Calibration/LumiAlCaRecoProducers
git cherry-pick e1ffa12420ea08c9d815de625d4ac60d8bb14936
git cherry-pick 8bb4def659b83fb36cce38ddb40f5847c07e0b27
git push my-cmssw +HEAD:pcc-per-roc-lumi_14_1_X

(then open the PR as usual)

@mandrenguyen
Copy link
Contributor

+1

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.

7 participants