Multiple issues fixed + --addPileupToSignal by CoralieNeubueser · Pull Request #32 · FCC-hh-framework/FCCSimJobs

CoralieNeubueser · 2018-06-01T07:13:46Z

positions using dd4hep decoder directly -> independent on some local 'privat' SW and urgent because recPositions does not work due to different root versions.
sliding window with "old" granularity + seeding window of half the size of final -> tested on pions, 100% efficiency
--addPileupToSignal functionality to create simuPU.. samples, these can then used as input for topo-clusters by --pileup ...
use --etaMax to specify the max. eta for physics simulations in the reco

…ith pu+noise thresholds

…addPileupToSignal

clementhelsens · 2018-06-04T10:17:30Z

Hello Coralie,
Is it in state to be reviewed?

CoralieNeubueser · 2018-06-04T13:30:33Z

Hi Clement,
yes it is. Please go ahead.
Cheers,
Coralie

…mJobs into addPileupToSignal

zaborowska

Hi Coralie,
I attach the inline comments.
Anna

zaborowska · 2018-06-06T07:25:35Z

  Detailed analysis and this scaling is done with FCC_calo_analysis_cpp toolkit.

- apply estimated noise levels at the cluster level for sliding window reconstruction or layer by layer for the topological clusters (**--addPileupNoise**).
+- apply estimated noise levels at the cluster level for sliding window reconstruction or per cell for the topological clusters (**--addPileupNoise**).


I thought for the topological clusters you do apply noise in a layer so that the correlation is properly taken into account (while to build the clusters you consider the values per cell)

Hi Anna, the pileup noise is added per cell dependent on its layer and eta, without any corrections due to correlations.

So the results you are showing do not take into account any correlations?
What about the study you did where you tried to parametrise the pileup noise for each layer as a function of the number of cells used from this layer (to build a cluster) ? This would at least take into account the correlations within one layer.

zaborowska · 2018-06-06T07:39:46Z

- cells that are later passed to the reconstruction (**--mergePileup**)

- or clusters that can be directly analysed (not yet supported)
+2.1. merge MinBias events (**--mergePileup**)


2.1. and 2.2 are both sub-cases of -- cells that are later passed to the reconstruction (**--mergePileup**).
Please put them there (you can make -- cells->2.1 cells and 2.1->2.1.1 and 2.2->2.1.2 and -- or clusters->2.2 or clusters

i don't see the need to add a case of 2.2, as long as it is not there. But i have added the 2.1.1/2. points.

it's to complete the picture. You yourself implemented in PR to FCCSW both the cell merging and the cluster merging.

zaborowska · 2018-06-06T07:41:48Z

- or clusters that can be directly analysed (not yet supported)
+2.1. merge MinBias events (**--mergePileup**)

+2.2. merge signal and PU events that are later passed to the reconstruction (**--addPileupToSignal**)


I believe we said there is no need to write out that file and it may be immediately passed to reconstruction?
But if you do use it separately, can you explain why do you need a separate config file instead of using overlayPileup.py? Isn't what's here enough?

yes, and i tried to produce them on the fly, but this took extemely long, such that the jobs would be killed.. so i added the intermediate step. For the new config file, this is way more easy, then trying to include this in the overlayPileup.py becasue the input is passed differently. you only have to define the process of your signal and the pu events are added. The input of the overlayPileup.py is the primary process itself, so it would not be trivial to add the signal in a consistent way.

So you are saying that you duplicated the whole configuration just because the flags --inName and --inSignalName are refering to pileup and signal, respectively? (in overlayPileup.py)
Why don't you rename them to inPileupName and inName? The pileup needs to be passed anyway in a special format as the input. This way you can have signal event passed as usual. (However make sure that mergePileup still works as expected)

zaborowska · 2018-06-06T07:46:01Z

@@ -0,0 +1,224 @@
+import argparse, os


I believe this config is not needed, see my prev comment

zaborowska · 2018-06-06T07:51:17Z

 simparser.add_argument('--winPhi', type=int, default=19, help='Size of the final cluster in phi')
 simparser.add_argument('--enThreshold', type=float, default=3., help='Energy threshold of the seeding clusters [GeV]')
-simparser.add_argument('--mu', type=int, default=1000, help='Pileup scenario')
+simparser.add_argument('--mu', type=int, default=0, help='Pileup scenario')


It is applied in the correction algorithm as a post step, so it does not influence the reconstruction process. Please leave the default value at 1000.

To me, the output was confusing, since it always writes out that pileup scenario: = 1000.. even though thats not true.

If you'd include the CorrectCluster algorithm (hence use inits/pileup.py), it would not be confusing. But ok, I can change that in the next PR.

zaborowska · 2018-06-06T08:08:12Z

            print "LHE: ", LHE
            print "jet pt: ", pt
-            job_dir += "etaTo0.5/" + str(pt) + "GeV/"
+            job_dir += "etaTo"+str(args.etaMax)+"/" + str(pt) + "GeV/"


this name doesn't correspond to the hard-coded values in lines 517-529.

true, i think Clement wanted to replace the hardcoded values. i just added this for the reco to be able to run on earlier produced simu files. but i think we can use the --etaMax to set this also for the physics simulations.
What do you say, @clementhelsens ?

zaborowska · 2018-06-06T08:13:57Z

            num_jobs = instatus
            warning("Directory contains only " + str(instatus) + " files, using all for the reconstruction")
+
+    if args.addPileupToSignal: 


Please try to combine it with lines 395-397. It should be fairly similar with the exception of running once again getInputFiles. Also it misses the comments to explain why this additional step is needed.

zaborowska · 2018-06-06T08:15:11Z

            common_fccsw_command += ' --detectorPath ' + path_to_FCCSW
-        if args.mergePileup:
-            common_fccsw_command += ' --pileup ' + str(args.pileup)
+        if args.physics:


Why do you say it is valid only for physics? Can't signal be a single particle?

zaborowska · 2018-06-06T08:16:39Z

            print "Name of the input file: ", infile
+            if args.addPileupToSignal :
+                pileupinfile = os.path.basename(pileup_input_files[i])
+                print "Name of the input file for pileup events: ", pileupinfile


it's not true, further down you pass all_pileup_inputs and not just this one file.

zaborowska · 2018-06-06T08:18:05Z

            if args.mergePileup:
                frun.write('%s --inName %s\n'%(common_fccsw_command, all_inputs))
+            elif args.addPileupToSignal:
+                frun.write('%s --inName %s --mu %i --inPileupFileNames %s\n'%( common_fccsw_command, input_files[i], args.pileup, all_pileup_inputs))


you already pass value of pileup in line 498

zaborowska · 2018-06-06T11:28:20Z

-import platform, os
-if platform.system()=='Darwin':
-    gSystem.Load(os.environ['/cvmfs/sft.cern.ch/lcg/releases/DD4hep/01-05-2396f/x86_64-slc6-gcc62-opt/lib/'])
+gSystem.Load(os.environ['/cvmfs/sft.cern.ch/lcg/releases/DD4hep/01-05-2396f/x86_64-slc6-gcc62-opt/lib/'])


Since if was unnecessary, you should remove that Load along with it.

CoralieNeubueser added 14 commits May 29, 2018 18:43

add pu events to signal in recTopoClusters

d83aa95

run clustering on overlays collections

b268060

SW binning back to ECal granularity

f039285

run topo-clusters on pileup+signal collections + electronics noise, w…

1c47f65

…ith pu+noise thresholds

rm not needed merge tool

78cd051

decoder for --recPositions from dd4hep

80b070c

adjust output path for --addPileupToSignal

b96a13c

recTopoClusters on PU+Signal simu

c2ed51a

Merge branch 'master' of github.com:CoralieNeubueser/FCCSimJobs into …

55c737b

…addPileupToSignal

seed cluster window set to half the final window size

d913399

merge conflicts solved

175d6cf

config for addPileupToSignal

7533525

prod mode

3c01515

bug fix

0d75ebf

CoralieNeubueser added 3 commits June 5, 2018 17:09

SW with HCal

4c41f16

add HCal cells to towers, also w/o noise

8238ee8

Merge branch 'addPileupToSignal' of github.com:CoralieNeubueser/FCCSi…

f9fdf01

…mJobs into addPileupToSignal

zaborowska suggested changes Jun 6, 2018

View reviewed changes

rm un-necessaries

b9b344d

zaborowska reviewed Jun 6, 2018

View reviewed changes

CoralieNeubueser added 3 commits June 6, 2018 15:03

addressed Annas comments

cc7d0f8

topo-clusters on pu+signal

ebdea4b

noise tested and running on PU configs

052d4e0

jhrdinka mentioned this pull request Jun 29, 2018

WIP:Digi track hits pile up merger #34

Open

jhrdinka mentioned this pull request Jul 9, 2018

WIP:Tracker digitization #35

Open

CoralieNeubueser added 3 commits August 23, 2018 15:13

updated with shuffle of input MinBias files before merging

2929fd7

add posibility to resegment HCal to DeltaEta=0.025

5cbf0fa

update warning, if local installation needed

135ccee

CoralieNeubueser added 6 commits August 23, 2018 15:31

add doku

41a0632

add doku

66c15b5

make sure that default window sizes used for default electron reco

72bf658

rm commented-out lines

ed625c6

add all collections

8e5d74e

set recalibrateBaseline=False in CreateCaloCells

01f2ae5

CoralieNeubueser closed this Aug 29, 2018

CoralieNeubueser deleted the addPileupToSignal branch August 30, 2018 12:48

Conversation

CoralieNeubueser commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clementhelsens commented Jun 4, 2018

Uh oh!

CoralieNeubueser commented Jun 4, 2018

Uh oh!

zaborowska left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CoralieNeubueser commented Jun 1, 2018 •

edited

Loading