LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
22 stars 20 forks source link

Trigger Scintillator #668

Closed latompkins closed 4 years ago

latompkins commented 4 years ago

Please collect here validation plots and information for the trigger scintillator. Links to code which generate the plots would be useful. Any bugs or specifically identified items for follow up should generate new issues.

Also, maybe it's worth having a trigger scintillator project like the other subdetectors?

bryngemark commented 4 years ago

Seeing strange features in the x distribution of sim hits up- and downstream of the target.

Will let Andrew comment on if this is spiraling particles or other known features.

val01-x_down.pdf val01-x_up.pdf

bryngemark commented 4 years ago

These figures were created using the trigger scintillator DQM processor currently only living in a branch of ldmx-sw: iss654 , run using Configuration/python/sample_trigScintDQM.py

And using 9 files : /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run102_seeds_204_205.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run104_seeds_208_209.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run105_seeds_210_211.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run106_seeds_212_213.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run113_seeds_226_227.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run11_seeds_22_23.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run120_seeds_240_241.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run126_seeds_252_253.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_01/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run129_seeds_258_259.root

awhitbeck commented 4 years ago

This does seem strange. I'll take a look at a few of the files now.

bryngemark commented 4 years ago

I checked if I could reproduce this in a small sample with no PN biasing, to start disentangling all effects. Indeed I see the same structure in my small (10k events) file (red histogram). This is for the upstream pad.

I also note that the beam spot profile is not very flat in x (as can be seen here) or y; it is strongly peaked (orders of magnitude more hits around what I assume is the beam spot center).

val01AndRef20x80-x_up

bryngemark commented 4 years ago

Ok, I ran one more test: took out the tagger (blue). I think it does look like the spikes between 0 and 20 disappear, which would support Andrew's hypothesis that it's something related to the tagger edges we see.

val01AndRef20x80_blueNoTag-x_up

Looking at the downstream pad (where the spike pattern is stronger), though, there seems to be some kind of modulation also without the tagger, but, the peaks are shifted a bit in x.

val01AndRef20x80_blueNoTag-x_down

bryngemark commented 4 years ago

Looking at the TrigScintHit collections in val02 data: 10 first files in /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_02/

It looks like there is some problem with the upstream pad reco/digi hit collection: even though it's there in the tree as "trigScintDigis", I get: No collection called trigScintDigis

For the tagger and downstream pads (trigScintDigisTag, trigScintDigisDn) there is no problem.

Inspecting the hit position histograms in the Tag collection, it looks like it has both tagger and upstream hits in it (see attached figures: the upstream pad should have hits close to origin, the tagger pad at large negative values in x and z).

Maybe a copy/paste error in the collection name?

I also advocate explicitly using "Up" in all upstream pad collection names, even if initial code didn't.

val02_tag_x val02_tag_z

awhitbeck commented 4 years ago

are these noise hits? (the hits at 0)

bryngemark commented 4 years ago

Hmmmm.... you mean that the x, y, z wouldn't be set for those?

awhitbeck commented 4 years ago

right, the positions are not set, because its impossible to do without also hardcoding the geometry into this code, which would be a pain to maintain:

https://github.com/LDMX-Software/ldmx-sw/blob/master/EventProc/src/TrigScintDigiProducer.cxx#L148-L177

However, the important thing is that the ids are set for noise and those should have a flat distribution. Also, for trigger scintillator we don't have information on the x position of the bar, so one should be careful using this information in reconstruction algorithms.

bryngemark commented 4 years ago

Ok, so, I don't think isNoise() is working the way it should. It is intended to flag pure noise hits.

val02_tag_id_noiseInRed

bryngemark commented 4 years ago

To follow up on the x distribution, here's a plot showing all the hit x distributions. Red: tagger pad Blue: upstream pad Orange: downstream pad Filled distributions are reco, sim is drawn with coloured lines only. No upstream collection in reco.

So the message here is that the structure in x in the two pads around the target, that seems to come from the tagging tracker planes (edges?), is still there in reco. Which makes me think it's not all super low energy stuff.

val02_simLinesRecoFill_tagRed-upBlue-downOrange_x


very technical details

10 input files: /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_02/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run0_seeds_0_1.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_02/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run100_seeds_200_201.root ... /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_02/4gev_1e_ecal_pn_ldmx-det-full-v12-fieldmap-magnet_run108_seeds_216_217.root

Histogramming code can be run using ldmx-app with the configs Configuration/python/sample_trigScintDQM.py (for sim hits) and Configuration/python/sample_trigScintHitDQM.py (for reco hits)

bryngemark commented 4 years ago

Ok, update is: I was using hit.isNoise() instead of hit.getNoise() {return isNoise_} , which made my selection of pure noise hits a completely random pile of junk, but didn't cause a compilation error. Using the proper method, the noise related distributions make sense (uniform noise hit distribution in channel ID, and the spike at the origin is confirmed to be pure noise hits).

Showing the PE distribution in the downstream pad as an example; hashed distribution is pure noise, the thicker black line is inclusive.

pe_down_wNoise

bryngemark commented 4 years ago

Heads-up about accessing the collection trigScintDigis (the upstream pad reco hits collection).

I mentioned that it exists in the tree, but that I couldn't access it with my DQM processor. The other collections (trigScintDigisTag, trigScintDigisDn) were fine.

In my code, I had a check: if ( !event.exists(hitCollectionName_.c_str()) ) { ... print some complaint and return }

Turns out this was triggered for trigScintDigis, and only for trigScintDigis. When I removed this check from my code, I could run over this collection just the same as for the other two.

This makes me wonder what is different between this collection and the other two.

omar-moreno commented 4 years ago

Both the upstream and downstream trigger scintillator arrays are not centered on the target. Currently, the bars are positioned starting from the bottom applying an offset to account for the mismatch in size between the trigger scintillator array and the target. An offset needs to be applied to account for this.

image

A fix is being put into issue #655.

omar-moreno commented 4 years ago

@awhitbeck @bryngemark Do you want to move the trigger pad closer to the tagger tracker? We could try placing it ~2 - 3 mm upstream of the first layer of the tagger tracker.

omar-moreno commented 4 years ago

After applying an offset (calculated using the size of the trigger scintillator pad), the arrays look centered.

I have not applied a fix to the tagger trigger scintillator pad.

image

bryngemark commented 4 years ago

A new comment here because I'm not sure the implications of my previous plots came through in the storm of information.

To me this looks like the beam doesn't come all the way through to the target? Or something is messed up with the trigger scintillator hit collections. Or something is messed up with my code.

So a second eye/explanations on this would be useful.

latompkins commented 4 years ago

Not to derail the comment thread here, but @bryngemark , can you present your validation results so far at the 4/13 SWAN meeting? Please include a summary of any additional issues raised or code changes needed. Thanks!

bryngemark commented 4 years ago

Sure!

bryngemark commented 4 years ago

Discussing with Andrew, we concluded that there is a mismatch between the gdml and the trigger scintillator hit reconstruction config in the number of bars/pad. Should be 50 both places but the config has 62 (remainder from some tests). I'll create an issue, hoping this can be fixed before the next validation sample round.

omar-moreno commented 4 years ago

Sounds good. I already ran a validation but it's no trouble to run another. I'll wait for your changes.

On Thu, Apr 16, 2020 at 2:32 PM bryngemark notifications@github.com wrote:

Discussing with Andrew, we concluded that there is a mismatch between the gdml and the trigger scintillator hit reconstruction config in the number of bars/pad. Should be 50 both places but the config has 62 (remainder from some tests). I'll create an issue, hoping this can be fixed before the next validation sample round.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/LDMX-Software/ldmx-sw/issues/668#issuecomment-614907623, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JMXBLZEDRXFI6HAFNBQLRM52OVANCNFSM4LUWUZSQ .

bryngemark commented 4 years ago

Ok, here's the set of validation plots on the reco hits. Shaded distributions are noise.

I think the only concerning one here is the ID distribution, which peaks at a weird place even though the y distributions are flat. (Andrew has also seen this.) Looks like a problem with the ID number assignment? Don't know if the digi/hit reconstruction code changed lately, if not maybe this is a problem with copy numnber in the gdml? Or if anyone knows what else has changed.

TrigScintHitDQM_id TrigScintHitDQM_n_hits TrigScintHitDQM_pe TrigScintHitDQM_x TrigScintHitDQM_y TrigScintHitDQM_z

bryngemark commented 4 years ago

Also, it's a little strange that the ID I extract which gives a peak (for hits with hit.getNoise() != 0 ), is not the same as for the noise hits (the dip is around channel 34, which is where Andrew found a peak). But I use the same method hit.getID()>>4 for filling the histograms.

I suppose the noise hit ID gets set during reco, but the sim hits get theirs in the sim step? On the other hand, for the dip to occur, the channels which are already hit need to be known, and associated to this number (34 or so) in the reco step, rather than at 2 or 3, where the peak is.

omar-moreno commented 4 years ago

Yes, sim hits get their IDs set by the SD. Noise hits are generated during the reconstruction stage so you would need to set the ID there.

If you are seeing issues with the IDs, taking a look at the SD is the place to start.

On Mon, May 4, 2020 at 12:07 PM bryngemark notifications@github.com wrote:

Also, it's a little strange that the ID I extract which gives a peak (for hits with hit.getNoise() != 0 ), is not the same as for the noise hits (the dip is around channel 34 which is where Andrew found a peak. But I use the same method hit.getID()>>4 for filling the histograms.

I suppose the noise hit ID gets set during reco, but the sim hits get theirs in the sim step? On the other hand, for the dip to occur, the channels which are already hit need to be known and associated to this number (34 or so) in the reco step, rather than at 2 or 3, where the peak is.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/LDMX-Software/ldmx-sw/issues/668#issuecomment-623648693, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JMXDDNCBUUQN4Y47WLNLRP4G73ANCNFSM4LUWUZSQ .

bryngemark commented 4 years ago

Looking into the gdml a bit, I suspect what is going on is that the combination of the upper trigger pad with the tagger, and the two others with the target, is what is messing up the copy number.

The tagger gets assigned copy number 1 (which is the channel ID we see in the tagger pad in the plot) in detector.gdml ; the target area has copy number 2 (again, like in the plot).

But the trigger pad channel IDs are defined through the copy numbers. They are supposed to be set individually for each of the 2x25 bars in the loops defining them. I suspect this definition gets overridden by the copy number assigned to them in detector.gdml

I started working towards testing this hypothesis but if it's too evident what the problem is, then I think we should focus on solving it instead. I can make verification a corollary as I go.

What is the underlying need behind combining the tagger/target with the trigger pad definitions? Anything I should keep in mind when/before pulling them out into their own gdmls?

omar-moreno commented 4 years ago

On Mon, May 4, 2020 at 2:53 PM bryngemark notifications@github.com wrote:

Looking into the gdml a bit, I suspect what is going on is that the combination of the upper trigger pad with the tagger, and the two others with the target, is what is messing up the copy number.

The tagger gets assigned copy number 1 (which is the channel ID we see in the tagger pad in the plot) in detector.gdml ; the target area has copy number 2 (again, like in the plot).

But the trigger pad channel IDs are defined through the copy numbers. They are supposed to be set individually for each of the 2x25 bars in the loops defining them. I suspect this definition gets overridden by the copy number assigned to them in detector.gdml

I started working towards testing this hypothesis but if it's too evident what the problem is, then I think we should focus on solving it instead. I can make verification a corollary as I go.

What is the underlying need behind combining the tagger/target with the trigger pad definitions?

Makes the biasing easier because they are defined as 1 region.

Anything I should be keep in mind when/before pulling them out into their own gdmls?

Make sure the trackers stay where they are at and don't break the biasing code :)

If it's easier, I can take a look and fix it. I just need to know how you want the ID's defined.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/LDMX-Software/ldmx-sw/issues/668#issuecomment-623728033, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JMXCQQHAT5OKZ5DXK4XLRP42OVANCNFSM4LUWUZSQ .

awhitbeck commented 4 years ago

my two cents is that the copy numbers are correct in the GDML and that something is inconsistent with the default CalorimeterSD class.

But actually, was this the same issue we had last summer that forced us to keep the geometry implemented in detector.gdml?

In any case, the place to start debugging is the CalorimeterSD. We should just make that class print a bug of things. If that is getting the subsystem and copy numbers correctly from the GDML, then the problem should be there. By the way, I saw the same thing in a collection of privately generated samples, so it is definitely reproducible with 10 inclusive electron events.

bryngemark commented 4 years ago

Ok. The CalorimeterSD code hasn't changed in like three years. The gdml changed last week.

As a quick check, when I edit the detector.gdml of the v12 detector, to not set the copy number of the target (while still setting them in target.gdml) by specifying physvol instead of physvol copynumber="2"

I get all channel IDs = 0 for non-noise hits in up-/downstream pads. So then they are not set at all. While previously they were all = 2.

So it is very clear to me that what happens is that the copy number gets (over)written in detector.gdml

The reason we didn't have this problem before was that the trigger scintillator pads were defined in detector.gdml. (But the problem we knew that this approach solved was different and had to do with overlapping volumes.) Omar, did you have an intermediate version where the trigger pads were defined in a separate gdml, that got validated wrt the channel IDs?

Otherwise I think we might need to either come up with a different scheme of setting the channel IDs (something closer to what other detectors do), or leave the trigger pads in detector.gdml (which is very ugly but might not be around for too long, if we are planning to abandon gdmls?).

omar-moreno commented 4 years ago

What ID scheme do you want so I can fix it? I doesn't make sense to go back to putting the trigger scintillator in the detector GDML when it can be fixed elsewhere.

On Tue, May 5, 2020, 10:34 AM bryngemark notifications@github.com wrote:

Ok. The CalorimeterSD code hasn't changed in like three years. The gdml changed last week.

As a quick check, when I edit the detector.gdml of the v12 detector, to not set the copy number of the target (while still setting them in target.gdml) by specifying

instead of

I get all channel IDs = 0 for non-noise hits in up-/downstream pads. So then they are not set at all. While previously they were all = 2.

So it is very clear to me that what happens is that the copy number gets overwritten in detector.gdml

The reason we didn't have this problem before was that the trigger scintillator pads were defined in detector.gdml. (But the problem we knew that this approach solved was different and had to do with overlapping volumes.) Omar, did you have an intermediate version where the trigger pads were defined in a separate gdml, that got validated wrt the channel IDs?

Otherwise I think we might need to either come up with a different scheme of setting the channel IDs (something closer to what other detectors do), or leave the trigger pads in detector.gdml (which is very ugly but might not be around for too long, if we are planning to abandon gdmls?).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/LDMX-Software/ldmx-sw/issues/668#issuecomment-624200176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4JMXAIJHFSVR5FLHUFR63RQBEZLANCNFSM4LUWUZSQ .

awhitbeck commented 4 years ago

we want the copy numbers to translate to the 'layer' value of the default detector id. Looks like the copy numbers are not getting into CalorimeterSD when they are defined in the target and tagger gdml files -- is that a correct interpretation of the problem?

bryngemark commented 4 years ago

Yes this is what I make of it.

If you Omar have a fix in mind then please go ahead and propose one, and we can review the PR to make sure we get the correct functionality implemented. To validate just run the trigScintHitDQM code and check the channel ID distributions. They should be flat with beam spot smearing. Or point me to a sample with the fix in, and I can do it.

bryngemark commented 4 years ago

Ran trigger pad DQM code on Omar's fix in branch iss559-dev (using a dedicated Sensitive Detector for the Trigger scintillators). And this fixed the ID problem.

This distribution is generated with one of my own configs, with

It's clear here that the offset in y leads to a shift in the hit distribution on the trigger pads. I'm not sure we catch all beam electrons with this geometry. An official sample might be needed to verify it, but, are we sure this is the behaviour we want?

TrigScintHitDQM_id

bryngemark commented 4 years ago

Also, actually, it looks a little weird that the actual hits go out to id 52. I think the numbering we want is 0,...,nChannels-1 ( = 2*nChannelsPerArray - 1)

awhitbeck commented 4 years ago

I just wanted to bring this pack to focus. Is the beam spot supposed to be at +5 mm? in y?

bryngemark commented 4 years ago

New round of validation: v06.

Files used:

/nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run0_seeds_0_1.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run107_seeds_214_215.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run109_seeds_218_219.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run10_seeds_20_21.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run11_seeds_22_23.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run130_seeds_260_261.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run137_seeds_274_275.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run138_seeds_276_277.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run13_seeds_26_27.root /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/4gev_1e_ecal_pn_val6_ldmx-det-v12_run148_seeds_296_297.root

commands, running over 10 first files: > ls /nfs/slac/g/ldmx/data/mc/v12/ecal_pn_val_06/* | head > filelist-v06.txt > for file in $(cat filelist-v06.txt ) ; do outFile=${file##*/} ; outFile="${outFile%.root}_digiDQM.root"; ldmx-app ../ldmx-sw/Configuration/python/examples/trigScintHitDQM.py $file $outFile ; done

and similarly for sim DQM, just replace digi or hit with sim. These distributions all look ok (and very similar to the reco ones, sharing the reco hit ("digi") ones here.

I found some minor bugs in the example python config for both reco hit and sim DQM, will push some fixes to allow anyone to run this properly.

As before, shaded distributions in the figures below represent noise hits.

The only outstanding question I have is the one about the beam center position in y. It's at +5mm, and the effect here is clearly visible (in the ID or y distributions): there are fewer hits at low ID/y, and after 5mm, it all looks flat. The sharp cut-off on the upper edge means that we are missing beam electrons.

So @omar-moreno, @awhitbeck, are we keeping this beam position? In that case, shouldn't we move the detector geometry to reflect this offset?

TrigScintHitDQM_id TrigScintHitDQM_x TrigScintHitDQM_y TrigScintHitDQM_z TrigScintHitDQM_n_hits TrigScintHitDQM_pe