Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OverlayTiming: shift hit time with offset #14

Closed
BrieucF opened this issue Dec 5, 2024 · 8 comments · Fixed by #15
Closed

OverlayTiming: shift hit time with offset #14

BrieucF opened this issue Dec 5, 2024 · 8 comments · Fixed by #15
Labels
enhancement New feature or request

Comments

@BrieucF
Copy link

BrieucF commented Dec 5, 2024

Hi @jmcarcell,

In the OverlayTiming algorithm, particles time is properly offset by the expected time between the physics collision and the BX from where the background particles are coming: https://github.com/key4hep/k4Reco/blob/main/k4Reco/Overlay/components/OverlayTiming.cpp#L260

Should'nt we do the same for the simTrackerHit (https://github.com/key4hep/k4Reco/blob/main/k4Reco/Overlay/components/OverlayTiming.cpp#L313) and for contributions (https://github.com/key4hep/k4Reco/blob/main/k4Reco/Overlay/components/OverlayTiming.cpp#L345)?

@BrieucF BrieucF added the enhancement New feature or request label Dec 5, 2024
@doloresgarcia
Copy link

The number of hits from the different background files (20) with 20 ns spacing
Screenshot 2024-12-05 at 13 04 12
The overall hit count
Screenshot 2024-12-05 at 13 04 05
vs the hit count resulting from the overlay algorithm
Screenshot 2024-12-05 at 11 14 48
With config:
overlay.NumberBackground = [20] overlay.TimeWindows = {"MCParticles": [0, 1000], "CDCHHits": [0, 400],"VTXDCollection": [0, 19], "VTXIBCollection": [0, 19], "VTXOBCollection": [0, 19] } overlay.Delta_t = 20

@jmcarcell
Copy link
Member

I'll make a PR to fix it; I must have missed it when porting because at least for the SimTrackerHit from LCIO the time is being set in the Marlin processor: https://github.com/iLCSoft/Overlay/blob/master/src/OverlayTiming.cc#L734.

@BrieucF
Copy link
Author

BrieucF commented Dec 6, 2024

Thanks Juan!

@doloresgarcia
Copy link

Hi all, I would like to reopen this issue. When I use the overlay with the following configuration:

PhysicsBX = 10
Delta_t = 20
NumberBackground =1

The timing of the overlay hits should be more or less uniformly distributed over the 400 ns since there are 20 bunches with a delta_t of 20 ns. However, that does not seem to be the case as shown in the plot below:
Image

The second problem is that the signal times also start at 0, while if there is an PhysicsBX larger than 1 it should be taken into account to adapt the time, as in this case the Physics bunch would happen after 10 background bunches:
Image

The scripts to reproduce this can the overlay script used can be found here:
/eos/experiment/fcc/ee/datasets/DC_tracking/Pythia/scratch/Zcard_CLD_background_IDEA_o1_v03_v4/1/

@andresailer andresailer reopened this Jan 28, 2025
@jmcarcell
Copy link
Member

jmcarcell commented Feb 10, 2025

I think you are not setting NBunchtrain, which controls how many bunches there are. The default is 1 and then you only get one bunch no matter what you put for the other parameters. Let's do an experiment, when running with these parameters

NBunchtrain = 10
Delta_t = 100
PhysicsBX = 1

I get:

where each peak corresponds to a bunch and they are separated by the 100 units of time that we set. Signal is scaled up so that it can be seen in the plot.

When running with

NBunchtrain = 10
Delta_t = 100
PhysicsBX = 3

I get:

PhysicsBX sets which bunch will have it's time unmodified, all the other bunches will have a time offset of N times Delta_t (with N being negative if they are the previous bunches before). So the time of the signal will always be unmodified, only the background changes. We could maybe look at other ways of setting the time if this is not what is wanted.

Also now thinking about this I don't know if PhysicsBX being indexed at 1 instead of 0 is a bit confusing, from the implementation it certainly is. That could be changed if people agree.

@doloresgarcia
Copy link

I understand, so I did have the NBunchtrain = 20 (forgot to add that above) but the plots I showed where for the hit times. I thought the time update happened to the time of the hits as well.
I get the same plots for the MC time then
Image

@jmcarcell
Copy link
Member

This should now be fixed in #19 so it will appear in tomorrow's nightlies. Please give it a try!

@doloresgarcia
Copy link

Yes, now I have the correct time for both hits and MC, I added a plot in #19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants