HcalPFCuts database table and relevant infrastructure #41128

igv4321 · 2023-03-22T15:24:12Z

PR description:

Added HcalPFCuts table for specifying seed and suppression thresholds for
PF Hcal objects. This table was discussed at

https://indico.cern.ch/event/1220970/contributions/5284605/attachments/2598482/4486203/HCAL_PFlowCutsSeeds_Feb24_2023.pdf

No changes expected in the output, as this table is not yet used by consumers.

PR validation:

Several private scripts were run to validate ascii and sql databse I/O of the new table.

cmsbuild · 2023-03-22T15:32:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34775

This PR adds an extra 108KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/Utilities/src/CondFormats.h modified in PR(s): Improved method for EB digi simulation #40855

cmsbuild · 2023-03-22T15:33:04Z

A new Pull Request was created by @igv4321 (Igor Volobouev) for master.

It involves the following packages:

CalibCalorimetry/HcalAlgos (alca)
CalibCalorimetry/HcalPlugins (alca)
CondCore/HcalPlugins (db)
CondCore/Utilities (db)
CondFormats/DataRecord (db, alca)
CondFormats/HcalObjects (db, alca)
CondTools/Hcal (db)

@cmsbuild, @tvami, @saumyaphor4252, @francescobrivio can you please review it and eventually sign? Thanks.
@rchatter, @tocheng, @argiro, @bsunanda, @thomreis, @simonepigazzini, @mmusich, @abdoulline, @mariadalfonso, @seemasharmafnal 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

abdoulline · 2023-03-22T15:52:27Z

@igv4321 thanks a lot, Igor!

https://github.com/cms-sw/cmssw/pull/41128/files#diff-569edf55a353f8a730365732c3099ca4cab4d2f2a48dc5dc7ce7366184d53442R4
😉

tvami · 2023-03-22T15:49:19Z

CondTools/Hcal/interface/HcalPFCutsHandler.h

@@ -0,0 +1,37 @@
+#ifndef HcalPFCutsHandler_h


Suggested change

#ifndef HcalPFCutsHandler_h

#ifndef CondTools_Hcal_HcalPFCutsHandler_h

tvami · 2023-03-22T15:49:32Z

CondTools/Hcal/interface/HcalPFCutsHandler.h

+#include "FWCore/Framework/interface/MakerMacros.h"
+#include "CondCore/PopCon/interface/PopConSourceHandler.h"
+
+#include "FWCore/Framework/interface/Event.h"
+#include "DataFormats/Common/interface/Handle.h"
+#include "FWCore/Framework/interface/EventSetup.h"
+// user include files
+#include "CondFormats/HcalObjects/interface/HcalPFCuts.h"
+#include "CondFormats/DataRecord/interface/HcalPFCutsRcd.h"
+#include "CalibCalorimetry/HcalAlgos/interface/HcalDbASCIIIO.h"


please alpha order these

tvami · 2023-03-22T15:52:18Z

CondTools/Hcal/plugins/HcalDumpConditions.cc

@@ -54,6 +54,7 @@ namespace edmtest {
          esConsumes<HcalPedestalWidths, HcalPedestalWidthsRcd>(edm::ESInputTag("", "effective"));
      tok_Gains = esConsumes<HcalGains, HcalGainsRcd>();


it would be better to have these all in the initializer part, but maybe for a follow-up PR

Indeed, but for now staying consistent with existing code that works should be ok

Yes this can be done in a follow-up PR

tvami · 2023-03-22T15:52:35Z

CondTools/Hcal/plugins/HcalPFCutsPopConAnalyzer.cc

+#include "CondTools/Hcal/interface/HcalPFCutsHandler.h"
+#include "FWCore/Framework/interface/MakerMacros.h"
+
+//typedef popcon::PopConAnalyzer<HcalPFCutsHandler> HcalPFCutsPopConAnalyzer;


remove commented out code

tvami · 2023-03-22T15:53:23Z

CondTools/Hcal/plugins/HcalPFCutsPopConAnalyzer.cc

+  void analyze(const edm::Event& ev, const edm::EventSetup& esetup) override {
+    //Using ES to get the data:
+
+    myDBObject = new HcalPFCuts(esetup.getData(m_tok));


instead of a new, could this be done as a unique_ptr?

There are 38 other *PopConAnalyzer.cc files in that directory, all using this type of initialization. This PR does not look to me like a good opportunity to replace popcon memory management model with something better.

Right but you are adding a new file here, I didnt ask to fix the rest of them (well now that you are bringing that up, maybe you should do that in a dedicated next PR...), I asked to have a new addition to be better. If you are not using the unique_ptr you need to make sure that myDBObject is deleted (otherwise this will do a memory leak). Can you point me to where that happens in the code?

I am not sure if it gets deleted... The pointer is passes to the PopCon system in the "endJob" method, and then PopCon does whatever is necessary to write the corresponding object or collection to the database. So even if there is a memory leak in a sense that the program does not itself delete every object it creates on the heap, all these objects have to be kept alive until the end of the job anyway, and the OS will do the right thing when the program exits.

Another plugin from HCAL HcalZDCLowGainFractionsPopConAnalyzer makes an explicit delete, see
https://github.com/cms-sw/cmssw/blob/master/CondTools/Hcal/plugins/HcalZDCLowGainFractionsPopConAnalyzer.cc#L21

HcalZDCLowGainFractions is a kind of dead end (not sure it was ever used) so may not serve as an example (?)

Most of the code in CondTools/Hcal/plugins is quite old, and was written when unique_ptr wasn't even part of the C++ standard. My preference would be to keep the code consistent with the old design, and if we get to fixing things, do it all at once.

tvami · 2023-03-22T15:54:14Z

CondTools/Hcal/src/HcalPFCutsHandler.cc

+
+void HcalPFCutsHandler::getNewObjects() {
+  //  edm::LogInfo   ("HcalPFCutsHandler")
+  std::cout << "------- " << m_name << " - > getNewObjects\n"


please use the LogInfo (or LogPrint etc) instead of teh cout, you also dont need std::endl when you use the MsgLogger functions

Again, this is just a boilerplate, with 41 pre-existing *Handler.cc in CondTools/Hcal/src out of which 35 print to cout. Not sure why this one should be different.

couts are strictly forbidden for new developments, so please change those, the rest of the package can be updated later

OK, prints to cout are removed in HcalPFCutsHandler.cc

tvami · 2023-03-22T15:58:24Z

Several private scripts were run to validate ascii and sql databse I/O of the new table.

Can you please include these as a unit test in this PR?

francescobrivio · 2023-03-22T16:41:23Z

@igv4321 since this PR is introducing a new CondObject I believe it's best if it's discussed at an AlCaDB meeting, maybe you or @abdoulline would be available to present next Monday (27^th March)?

francescobrivio · 2023-03-22T16:42:42Z

Another question: in the PR description there is no mention of the need for Backports, will this be needed for online data taking this year?

abdoulline · 2023-03-22T17:06:01Z

Another question: in the PR description there is no mention of the need for Backports, will this be needed for online data taking this year?

Hi @francescobrivio

(1) I can present the things at the next AlCaDB meeting

(2) as to the backporting - we'd like to have it backported for 2023 data taking, if circumstances would allow,
not sure whether is can be done now = so late in 13_0_X (patch) as using these conditions (instead of actual configurables) would require some (albeit minimal) code & configuration modification in PFlow and EGamma packages (-> HLT menus)

francescobrivio · 2023-03-22T20:19:15Z

Thank you Salavat!

(1) I can present the things at the next AlCaDB meeting

The agenda is ready https://indico.cern.ch/event/1269033/#65-hcal-pfcuts-db-conditions, let us know in case you prefer a differnt title (and please note that the agenda is still under construction)

(2) as to the backporting - we'd like to have it backported for 2023 data taking, if circumstances would allow, not sure whether is can be done now = so late in 13_0_X (patch) as using these conditions (instead of actual configurables) would require some (albeit minimal) code & configuration modification in PFlow and EGamma packages (-> HLT menus)

Indeed you are right, the 13_0_X cycle (for 2023 data-taking) is kinda closed to major updates like this one: it has to be discussed with PPD L1s and release managers (FYI @perrotta @rappoccio)

igv4321 · 2023-03-23T04:35:41Z

Unit test added, as requested

cmsbuild · 2023-03-23T04:38:35Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34795

This PR adds an extra 440KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/Utilities/src/CondFormats.h modified in PR(s): Improved method for EB digi simulation #40855

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34795/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34795/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2023-03-23T04:52:58Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34796

This PR adds an extra 440KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/Utilities/src/CondFormats.h modified in PR(s): Improved method for EB digi simulation #40855

cmsbuild · 2023-03-23T04:53:26Z

Pull request #41128 was updated. @cmsbuild, @tvami, @saumyaphor4252, @francescobrivio can you please check and sign again.

tvami · 2023-03-30T17:58:17Z

@cmsbuild , please test

cmsbuild · 2023-03-30T18:01:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41128/34962

This PR adds an extra 112KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File CondCore/Utilities/src/CondFormats.h modified in PR(s): Improved method for EB digi simulation #40855

cmsbuild · 2023-03-30T18:02:09Z

Pull request #41128 was updated. @tvami, @saumyaphor4252, @francescobrivio can you please check and sign again.

tvami · 2023-03-30T19:09:43Z

+1

unit test testing the new rcd is passed

* PR according to the desc and the follow-up to the review

cmsbuild · 2023-03-30T19:10:12Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

rappoccio · 2023-03-30T20:27:51Z

+1

cmsbuild · 2023-03-30T21:00:11Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-faea2f/31699/summary.html
COMMIT: fdf839b
CMSSW: CMSSW_13_1_X_2023-03-30-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41128/31699/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 5 lines from the logs
Reco comparison results: 12 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3554236
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3554208
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 213 log files, 164 edm output root files, 49 DQM output files
TriggerResults: no differences found

cmsbuild added this to the CMSSW_13_1_X milestone Mar 22, 2023

cmsbuild added alca-pending db-pending pending-signatures tests-pending orp-pending code-checks-pending labels Mar 22, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 22, 2023

tvami reviewed Mar 22, 2023

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-approved labels Mar 23, 2023

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Mar 23, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 23, 2023

cmsbuild added tests-pending code-checks-pending and removed tests-rejected requires-external code-checks-approved labels Mar 30, 2023

cmsbuild added tests-started and removed tests-pending labels Mar 30, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 30, 2023

cmsbuild added alca-approved fully-signed db-approved and removed alca-pending db-pending pending-signatures labels Mar 30, 2023

cmsbuild added orp-approved and removed orp-pending labels Mar 30, 2023

cmsbuild added tests-approved and removed tests-started labels Mar 30, 2023

cmsbuild merged commit 60499d5 into cms-sw:master Mar 30, 2023

saumyaphor4252 mentioned this pull request Apr 12, 2023

Include HcalPFCuts Rcd and update SiPixel Dynamic Inefficiency tags in MC GTs #41327

Merged

malbouis mentioned this pull request May 9, 2023

[12_4_X] New HB thresholds for 2022 data rereco #41581

Merged

swagata87 mentioned this pull request Oct 16, 2023

Adapt PF code to read Hcal thresholds from GT #43025

Merged

	#ifndef HcalPFCutsHandler_h
	#ifndef CondTools_Hcal_HcalPFCutsHandler_h

		@@ -54,6 +54,7 @@ namespace edmtest {
		esConsumes<HcalPedestalWidths, HcalPedestalWidthsRcd>(edm::ESInputTag("", "effective"));
		tok_Gains = esConsumes<HcalGains, HcalGainsRcd>();

HcalPFCuts database table and relevant infrastructure #41128

HcalPFCuts database table and relevant infrastructure #41128

Uh oh!

Conversation

igv4321 commented Mar 22, 2023

PR description:

PR validation:

Uh oh!

cmsbuild commented Mar 22, 2023

Uh oh!

cmsbuild commented Mar 22, 2023

Uh oh!

abdoulline commented Mar 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvami commented Mar 22, 2023

Uh oh!

francescobrivio commented Mar 22, 2023

Uh oh!

francescobrivio commented Mar 22, 2023

Uh oh!

abdoulline commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francescobrivio commented Mar 22, 2023

Uh oh!

igv4321 commented Mar 23, 2023

Uh oh!

cmsbuild commented Mar 23, 2023

Uh oh!

cmsbuild commented Mar 23, 2023

Uh oh!

cmsbuild commented Mar 23, 2023

Uh oh!

tvami commented Mar 30, 2023

Uh oh!

cmsbuild commented Mar 30, 2023

Uh oh!

cmsbuild commented Mar 30, 2023

Uh oh!

tvami commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

abdoulline commented Mar 22, 2023 •

edited

Loading

tvami commented Mar 30, 2023 •

edited

Loading