Infrastructure for HCAL channel-dependent pulse shapes using database configurations #49556
Conversation
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49556/47076
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
cmsbuild
added
code-checks-rejected
code-checks-pending
and removed
code-checks-pending
code-checks-rejected
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49556/47077
|
|
A new Pull Request was created by @igv4321 for master. It involves the following packages:
@Alejandro1400, @JanChyczynski, @arunhep, @atpathak, @cmsbuild, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
you can make yourself a PR to this repository https://github.com/cms-data/CondTools-Hcal and then the two PRs can be tested together. |
|
@igv4321 do the changes need to be propagated to the heterogeneous version of the code under |
It would be nice to propagate them, but HCAL DPG does not have an alpaka expert at the moment. But even if just for offline, this capability is very useful. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49556/47083
|
|
Pull request #49556 was updated. @Alejandro1400, @JanChyczynski, @arunhep, @atpathak, @cmsbuild, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @srimanob can you please check and sign again. |
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
requires-external
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49556/47239
|
|
Pull request #49556 was updated. @Alejandro1400, @JanChyczynski, @Moanwar, @arunhep, @atpathak, @cmsbuild, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @srimanob can you please check and sign again. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-49556/47240
|
|
Pull request #49556 was updated. @Alejandro1400, @JanChyczynski, @Moanwar, @arunhep, @atpathak, @cmsbuild, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @srimanob can you please check and sign again. |
|
please test with cms-data/CondTools-Hcal#2 |
|
+1 Size: This PR adds an extra 16KB 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 SummaryThere are some workflows for which there are errors in the baseline: The workflows 2024.0070001 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons Summary:
|
@igv4321 could you please report a summary of those tests and their results in this github thread? |
|
There is a set of instructions describing the format of pulse shape representations and the tools used to make mysql files out of them in the file CondTools/Hcal/test/readme_HcalInterpolatedPulseMap.txt. It was checked that all operations described in these instructions work as intended. |
|
Hi all, just a ping on this PR. HCAL DPG would very much like to see this move forward. |
|
I'm working on the review already. I'll post first comments soon |
JanChyczynski
left a comment
JanChyczynski
left a comment
There was a problem hiding this comment.
A partial review for now, I have mostly only looked at the CondTools package.
There was a problem hiding this comment.
There are several problems with this file (and related HcalPulseDelaysHandler):
- Defnining duplicate members
m_sourceandm_populatorresults in 2 separate instances of these fields (e.g.HcalPulseDelaysPopConAnalyzer::m_sourceandPopConAnalyzer::m_source). Its not a proper use of inheritance and is dangerous and error-prone with any future changes (eg. access tom_sourcefrom the basePopConAnalyzerwill access a different instance of the source). - Re-implementing
write()is a (potentially unecessary) code duplication myDBObjectshould not be defined as a raw pointer. RunningHcalPulseDelaysPopConAnalyzer.analyzemutliple times (i.e. for multiple events) would result with a memory leak. It would be much better to useunique_ptrandstd::movewhen callingsource.initObject()to transfer the ownership. AccordinglyinitObject()should take aunique_ptras the parameter to express taking ownership.
I've noticed 40 other PopConAnalyzers in CondTools/Hcal are defined this way. It is beyond the scope of this PR to change the existing classes, so I would suggest to implement the new one in a better way and add a comment to all the existing classes with this design mentioning the problem so this pattern is not copied in the future and the newly added class should be referenced instead. It would be good to add a github issue and mention it in the comment. If you want I can add the issue soon.
3. is easy to fix by changing myDbObject and the parameter of initObject to unique_ptr .
Because m_source, m_populator, write and endJob are private is tricky to fix point 1. and 2. My idea is to move source.initObject(myDBObject); to .analyze(...) . This would allow not to redefine write() and using source() getter would allow not to redefine m_source.
But I need you to confirm that this would result in the same logic and behaviour. The difference I see is that it would be then run for every event instead of once, in endJob() . From what I understand HcalPulseDelaysPopConAnalyzer (and other PopConAnalyzers with the same pattern) is not meant to be run for multiple events (as multiple execution of anaylze would result in a memory leak), thus this change of behaviour resulting from moving initObject to analyze is not a problem. If this is not appropriate, let me know and I'll present an alternative solution.
Below I provide a simplified DRAFT implementation with a TODO comment about the aforementioned doubt:
//note: simplified implementation, the final implementation needs to inherit from a template base class implementing this pattern.
class HcalPulseDelaysPopConAnalyzer : public popcon::PopConAnalyzer<HcalPulseDelaysHandler> {
public:
typedef HcalPulseDelaysHandler SourceHandler;
HcalPulseDelaysPopConAnalyzer(const edm::ParameterSet& pset)
: popcon::PopConAnalyzer<HcalPulseDelaysHandler>(pset),
m_token(esConsumes<HcalPulseDelays, HcalPulseDelaysRcd>()) {}
private:
void analyze(const edm::Event& ev, const edm::EventSetup& esetup) override {
// TODO confirm that all use cases are meant to have this run only once:
// then throw exception if run more than once
//Using ES to get the data:
source().initObject(std::make_unique<HcalPulseDelays>(esetup.getData(m_token)));
}
private:
edm::ESGetToken<HcalPulseDelays, HcalPulseDelaysRcd> m_token;
};Because the same problematic patter occurs in 40 other PopConAnalyzers it would be best to prepare a base class for these problematic classes with the fix implemented. I've prepared a draft PR with the implementation of that base class and an example of using it: #49905
Early next week I can finish and publish that PR and then you can base your classes on it. If you prefer you can also cherry pick the commit with the implementation of the base class (git cherry-pick https://github.com/cms-AlCaDB/cmssw.git <commit hash> ). Let me know what works better.
7 participants
PR description:
Adding infrastructure for modeling HCAL front-end pulse shapes channel-by-channel. This PR superseeds #45995
Naturally, db-based pulse shapes are not enabled yet (they are not in the CMS database), so no changes in validation expected at the moment.
PR validation:
The usual matrix tests were run. The facilities for reading/writing the database tables were privately tested. I need somebody with right permissions to take the file CondTools/Hcal/test/hcalpulsedelays.txt and copy it into a directory where it can be found by "edmFileInPath". Then I will be able to turn on the unit test for the database table of pulse height delays.