The effect of module sum split over multiple towers on physics objects #472

mhassans · 2021-05-21T23:52:10Z

PR description:

In trigger primitive generator (TPG) of the HGCal, the total energy of a module (module sum) is split over multiple towers overlapping with it. We are investigating the effect of this splitting on the variables associated to physical objects, such as the position/energy resolution of e/gamma/jets.

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

jbsauvan · 2021-06-11T13:00:15Z

Hi @mhassans
There are some external commits that should not be there. It seems this is coming from a merge.
In general the best is to do rebases and not merges in order to keep a clean history.

mhassans · 2021-06-11T14:44:29Z

Hi @mhassans
There are some external commits that should not be there. It seems this is coming from a merge.
In general the best is to do rebases and not merges in order to keep a clean history.

Hi @jbsauvan
Yes, sorry for that. Should I try undoing this merge to make it cleaner? Or you only meant for future I should do a rebase instead of merge?

jbsauvan · 2021-06-11T15:22:21Z

I would be good to fix it as I will ask you at the very end, before merging, to squash all the commits.
You can probably fix it with an interactive rebase and pick only your commits.

mhassans · 2021-06-11T17:01:36Z

I would be good to fix it as I will ask you at the very end, before merging, to squash all the commits.
You can probably fix it with an interactive rebase and pick only your commits.

Thanks, it should be fixed now.

DataFormats/L1THGCal/interface/HGCalTowerMap.h

DataFormats/L1THGCal/src/HGCalTowerMap.cc

L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc

jbsauvan · 2021-06-30T15:25:15Z

L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc

+    for (int i=1; i<=std::stoi(result.at(4)); i++){
+      towerEta = 2 + std::stoi(result.at(3*i+2)); // shift by two to avoid negative eta
+      towerPhi = (std::stoi(result.at(3*i+3)) + sector*int(nBinsPhi_)/3 + int(nBinsPhi_)/2) % int(nBinsPhi_);//move to the correct sector
+      if(zside==1){
+        towerPhi = (3*int(nBinsPhi_)/2 - towerPhi - 1) % int(nBinsPhi_); //correct x -> -x in z>0
+      }
+      binIDandShares.insert( {l1t::HGCalTowerID(doNose_, zside, towerEta, towerPhi).rawId(),  std::stod(result.at(3*i+4))/splitDivisor } );
+    }


All the hardcoded numbers should be implemented as named constants, otherwise it is difficult to understand what they mean exactly

L1Trigger/L1THGCal/interface/HGCalTriggerTowerGeometryHelper.h

L1Trigger/L1THGCal/python/hgcalTowerMapProducer_cfi.py

L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc

jbsauvan · 2021-09-21T08:51:44Z

Hello @mhassans
Are there any news regarding this PR?
Thanks!

mhassans · 2021-09-21T09:30:24Z

Hello @mhassans
Are there any news regarding this PR?
Thanks!

Hi Jean-Baptiste,

Sorry for delay on this. I will start working on this soon. I will also send you an email explaining the reasons for this delay.

Thanks!

jbsauvan

Thanks @mhassans
Here are some additional comments.

DataFormats/L1THGCal/interface/HGCalTowerID.h

L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc

mhassans · 2021-10-28T13:49:27Z

Hi @jbsauvan

Thanks for your comments. I implemented all in the code. Please let me know if the code needs any further changes.

jbsauvan

Thanks a lot @mhassans
I just have one remaining minor naming comment below. Once implemented, could you:

rebase your work on top of the latest devel branch (currently CMSSW_12_0_0_pre3). Given that you have a large number of commits, it might be simpler to squash them first and rebase.
run scram b code-format and commit the proposed changes

jbsauvan · 2021-11-01T13:24:08Z

L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc

-            std::string substr;
-            std::getline(ss, substr, ' ');
-            lineSplitted.push_back(substr);
+      int NumOfWordsInThisRow = 0;


Variable names should not start with a capital letter (usually reserved for class names).
So should rather be numOfWordsInThisRow

Thanks a lot @mhassans I just have one remaining minor naming comment below. Once implemented, could you:

rebase your work on top of the latest devel branch (currently CMSSW_12_0_0_pre3). Given that you have a large number of commits, it might be simpler to squash them first and rebase.

run scram b code-format and commit the proposed changes

Thanks @jbsauvan

I implemented the comment and then squashed the commits. Could you please confirm if the rebasing can be done in the following way?

Create a branch from the latest CMSSW version:
git checkout -b my_CMSSW_12_0_0_pre3 pfcaldev/hgc-tpg-devel-CMSSW_12_0_0_pre3
Then update it with origin (pfcaldev):
git pull pfcaldev hgc-tpg-devel-CMSSW_12_0_0_pre3
Back to my branch:
git checkout moh-modSplit
Finally rebase:
git rebase my_CMSSW_12_0_0_pre3
Then force push it to my branch:
git push myRemote +moh-modSplit

Hi @mhassans
That looks correct. In case you want to check that the rebase will pick the commit you want, you can do an interactive rebase (with -i)

@jbsauvan
There are some merging conflicts after I did the rebase (please see the attached screenshot). Should I resolve it by choosing lines from the newest release (12_0_0_pre3), or the conflicts are not expected?

There should be conflict only in the files that you modified, which doesn't seem to be the case here. So these conflicts are not expected. I'll have a look.

It seems that the rebase wants to apply a bunch of commits unrelated to this PR. The simplest then is probably to cherry-pick your single commit. Or you can still do the rebase but do it interactively (with -i) and remove all the commits except your commit.

By the way, I just modified the base branch of the PR to the latest devel branch (this is why there are now conflicts). When you push the rebased branch, it should be able to merge again.

@jbsauvan Thanks! The rebasing went well this time and I also did code-format check. So please let me know about the next step if everything looks fine.

jbsauvan · 2021-11-02T14:50:14Z

Thanks @mhassans
There are some includes to be fixed. I got these compilation errors:

In file included from /grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc:2:                                                                                                                                                                 
/grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/interface/HGCalTriggerTowerGeometryHelper.h:18:10: fatal error: L1Trigger/L1THGCal/interface/HGCalModuleDetId.h: No such file or directory                                                                                      
   18 | #include "L1Trigger/L1THGCal/interface/HGCalModuleDetId.h"                                                                                                                                                                                                                                                                                    
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
>> Compiling  /grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/src/backend/HGCalClusteringDummyImpl.cc                                                                                                                                                                           
>> Compiling  /grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/src/backend/HGCalClusteringImpl.cc                                                                                                                                                                                
In file included from /grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/src/HGCalTriggerTowerGeometryHelper.cc:2:                                                                                                                                                                 
/grid_mnt/vol_home/llr/cms/sauvan/01-Projects/2105-XX_Review-modulesum-split/CMSSW_12_0_0_pre3/src/L1Trigger/L1THGCal/interface/HGCalTriggerTowerGeometryHelper.h:18:10: fatal error: L1Trigger/L1THGCal/interface/HGCalModuleDetId.h: No such file or directory                                                                                      
   18 | #include "L1Trigger/L1THGCal/interface/HGCalModuleDetId.h"                                                                                                                                                                                                                                                                                    
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This ModuleDetId class has now been moved to DataFormats/ForwardDetId/interface/HGCalTriggerModuleDetId.h

mhassans · 2021-11-02T15:50:44Z

@jbsauvan Ah I see. So it just needs renaming the path and the class names wherever used. This means changing the old name (HGCalModuleDetId) to the new name (HGCalTriggerModuleDetId).

One more question: How can I un-hide DataFormats/ForwardDetId in my local machine?

jbsauvan · 2021-11-02T16:00:33Z

@jbsauvan Ah I see. So it just needs renaming the path and the class names wherever used. This means changing the old name (HGCalModuleDetId) to the new name (HGCalTriggerModuleDetId).

In principle yes.

One more question: How can I un-hide DataFormats/ForwardDetId in my local machine?

You can git cms-addpkg DataFormats/ForwardDetId

mhassans · 2021-11-02T16:37:58Z

Thanks @jbsauvan . I pushed the changes. It should work now.

jbsauvan · 2021-11-03T16:07:17Z

Thanks @mhassans

Performing some last tests, it seems that the test configs doesn't run, it misses the splitModuleSum parameter in the default config. Can you add splitModuleSum=cms.bool(False) in the L1TTriggerTowerConfig_etaphi PSet.
Also you'll need to rerun code-format

mhassans · 2021-11-03T16:45:14Z

Thanks @mhassans

Performing some last tests, it seems that the test configs doesn't run, it misses the splitModuleSum parameter in the default config. Can you add splitModuleSum=cms.bool(False) in the L1TTriggerTowerConfig_etaphi PSet.

Also you'll need to rerun code-format

Thanks @jbsauvan . I made the changes and re-ran the code-format.

jbsauvan · 2021-11-04T09:17:14Z

Default config keeps the tower distributions the same:

[Red=reference, Blue=this PR]
Switching on the module splitting, more towers are produced containing less energy:

-> Behaves as expected.

mhassans force-pushed the moh-modSplit branch from 54c1049 to ac82f1a Compare June 11, 2021 16:56

mhassans marked this pull request as ready for review June 28, 2021 10:43

jbsauvan reviewed Jun 30, 2021

View reviewed changes

jbsauvan reviewed Jul 12, 2021

View reviewed changes

jbsauvan reviewed Oct 21, 2021

View reviewed changes

jbsauvan reviewed Nov 1, 2021

View reviewed changes

mhassans force-pushed the moh-modSplit branch from 4a91fc1 to 89b2537 Compare November 1, 2021 17:59

jbsauvan changed the base branch from hgc-tpg-devel-CMSSW_12_0_0_pre1 to hgc-tpg-devel-CMSSW_12_0_0_pre3 November 2, 2021 13:02

Add option to split module sums over towers

2b66b16

mhassans force-pushed the moh-modSplit branch from 89b2537 to 2b66b16 Compare November 2, 2021 14:28

code-format check

6b0946b

replace HGCalModuleDetID with HGCalTriggerModuleDetId

9aea0d4

Mohammadhassan Hassanshahi added 2 commits November 3, 2021 17:36

add SplitModuleSum to the default config

f5a0a63

checked for code-format

5965a91

jbsauvan merged commit c4b1a1b into PFCal-dev:hgc-tpg-devel-CMSSW_12_0_0_pre3 Nov 4, 2021

jbsauvan mentioned this pull request Nov 4, 2021

[Backport 11_3_0] Add option to split module sums over towers #506

Merged

This was referenced Jul 12, 2023

[HGCAL trigger] Module splitting and updated seeding normalization cms-sw/cmssw#42246

Merged

Integration of updated seeding normalization and module splitting hgc-tpg/cmssw#64

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The effect of module sum split over multiple towers on physics objects #472

The effect of module sum split over multiple towers on physics objects #472

mhassans commented May 21, 2021

jbsauvan commented Jun 11, 2021

mhassans commented Jun 11, 2021

jbsauvan commented Jun 11, 2021

mhassans commented Jun 11, 2021

jbsauvan Jun 30, 2021

jbsauvan commented Sep 21, 2021

mhassans commented Sep 21, 2021

jbsauvan left a comment

mhassans commented Oct 28, 2021

jbsauvan left a comment

jbsauvan Nov 1, 2021

mhassans Nov 1, 2021

jbsauvan Nov 2, 2021

mhassans Nov 2, 2021

jbsauvan Nov 2, 2021

jbsauvan Nov 2, 2021

jbsauvan Nov 2, 2021

mhassans Nov 2, 2021

jbsauvan commented Nov 2, 2021

mhassans commented Nov 2, 2021

jbsauvan commented Nov 2, 2021

mhassans commented Nov 2, 2021

jbsauvan commented Nov 3, 2021

mhassans commented Nov 3, 2021

jbsauvan commented Nov 4, 2021

The effect of module sum split over multiple towers on physics objects #472

The effect of module sum split over multiple towers on physics objects #472

Conversation

mhassans commented May 21, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

jbsauvan commented Jun 11, 2021

mhassans commented Jun 11, 2021

jbsauvan commented Jun 11, 2021

mhassans commented Jun 11, 2021

Choose a reason for hiding this comment

jbsauvan commented Sep 21, 2021

mhassans commented Sep 21, 2021

jbsauvan left a comment

Choose a reason for hiding this comment

mhassans commented Oct 28, 2021

jbsauvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbsauvan commented Nov 2, 2021

mhassans commented Nov 2, 2021

jbsauvan commented Nov 2, 2021

mhassans commented Nov 2, 2021

jbsauvan commented Nov 3, 2021

mhassans commented Nov 3, 2021

jbsauvan commented Nov 4, 2021