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

Iss1053 -- add "no spacing" capability to MC readout #1057

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bloodyyugo
Copy link
Contributor

We want to be able to read out the MC+pulser overlay event without putting a lot empty bunches between the events so that the readout system can handle it without piling up all the hits in time.

The hack I did was:

-- in ReadoutDataManager, which carries the global time, each event gets incremented by 250 bunches (500ns) instead of the single bunch-per-event in the spaced MC
-- for the sim readout, digitization, GTP cluster making and triggering, I created a bunch of "NoSpacing" classes mostly copied from/named similarly to their corresponding classes used for the spaced MC. I changed the timing of where the digitized pulses were placed to make them all be around the time given in readoutTime, removing some latencies.
-------------- I may revisit this now that I understand the procedure better. We may just be able to adjust the numbers (and maybe a little code) to get this to work with existing code.

This is not ready for primetime yet.

…er.java which I use to make comparison plots and should be in master at some point
@bloodyyugo bloodyyugo linked an issue Dec 5, 2024 that may be closed by this pull request
…readout; remove standalone no-spacing drivers
@bloodyyugo
Copy link
Contributor Author

Took a couple different turns here but settled on just:

  1. add settable boolean doNoSpacing to ReadoutDriver.java (all drivers inherit from)
  2. add settable int effectiveBunches to ReadoutDataManager.java; this sets time increment/even to effectiveBunches*2ns....set to 1 for spaced readout, 250 for unspaced
  3. small modifications to readout drivers to work with the unspaced readout; in particular, instead of using any "internal" clocks, they just get the global time from ReadoutDataManager

@bloodyyugo
Copy link
Contributor Author

To test this, I ran over an unspaced tritrig file: /fs/ddn/sdf/group/hps/mc/tritrig/slic/3pt74/20um120nA/HPS_Run2021Pass1_v4newBot/tritrig_3pt74_20um120nA_rot_863.slcio

...and the corresponding spaced file (both provided by SarahG):
/fs/ddn/sdf/group/hps/users/sgaiser/mc/spaced_only_out/tritrig_spaced_3pt74_HPS_Run2021Pass1_v4newBot_863.slcio

I ran 3 versions of readout, with noise turned off so I could compare on equal footing, and found this many triggers:
new unspaced readout (doNoSpacing = true): 922 triggers
new spaced readout (doNoSpacing = false): 883
current master spaced readout: 883
..so, the old spaced readout (current master) and the new code (supposed to just add no spacing functionality) show the same triggers. That is good!

We find ~5% more triggers with the unspaced readout. Digging through lots of print statements in the spaced/unspaced readout, these are mostly (all? all that I checked) have the ecal gtp cluster that would trigger a 4ns tick earlier than the trigger gets the hodoscope patterns. If there is not both a gtp cluster and a hodoscope pattern in the same 4ns tick, the code returns without triggering. The hodo patterns are actually available to the trigger for an extended period (many 4ns ticks) but the gtp clusters only a single one.

I think it's weird that the trigger requires both the hodo pattern and gtp cluster collections exist at the same time period, since I'm pretty sure one of singles2 or singles3 doesn't require the hodoscope. Is this really reproducing what the real trigger does?

@tongtongcao
Copy link
Contributor

To test this, I ran over an unspaced tritrig file: /fs/ddn/sdf/group/hps/mc/tritrig/slic/3pt74/20um120nA/HPS_Run2021Pass1_v4newBot/tritrig_3pt74_20um120nA_rot_863.slcio

...and the corresponding spaced file (both provided by SarahG): /fs/ddn/sdf/group/hps/users/sgaiser/mc/spaced_only_out/tritrig_spaced_3pt74_HPS_Run2021Pass1_v4newBot_863.slcio

I ran 3 versions of readout, with noise turned off so I could compare on equal footing, and found this many triggers: new unspaced readout (doNoSpacing = true): 922 triggers new spaced readout (doNoSpacing = false): 883 current master spaced readout: 883 ..so, the old spaced readout (current master) and the new code (supposed to just add no spacing functionality) show the same triggers. That is good!

We find ~5% more triggers with the unspaced readout. Digging through lots of print statements in the spaced/unspaced readout, these are mostly (all? all that I checked) have the ecal gtp cluster that would trigger a 4ns tick earlier than the trigger gets the hodoscope patterns. If there is not both a gtp cluster and a hodoscope pattern in the same 4ns tick, the code returns without triggering. The hodo patterns are actually available to the trigger for an extended period (many 4ns ticks) but the gtp clusters only a single one.

I think it's weird that the trigger requires both the hodo pattern and gtp cluster collections exist at the same time period, since I'm pretty sure one of singles2 or singles3 doesn't require the hodoscope. Is this really reproducing what the real trigger does?

@tongtongcao tongtongcao closed this Dec 6, 2024
@tongtongcao
Copy link
Contributor

tongtongcao commented Dec 6, 2024

For each clock cycle, all hodoscope hits in a range of time (before and after the clock cycle) are collected to build hodoscope patterns. Slides 8&9 in the attached file explain how the hodoscope pattern driver works.
Updates of the Readout System for 2019 MC.pdf

@tongtongcao tongtongcao reopened this Dec 6, 2024
@bloodyyugo
Copy link
Contributor Author

Yeah, I understand this. All I am saying is that, for those ~5% of inserted MC signal events, the time of the gtp cluster that would make a good trigger is before that hodoscope pattern time window and thus doesn't actually make a trigger. And the unspaced readout, with simpler time structure counts there as good triggers so unspaced may overestimate overall efficiency (and thus rates) by this ~5%.

However, I want to make sure that this is in line with what we expect for real triggers?
Do we expect to miss 5% of good triggers because the positron cluster ends up in a time bucket that's too early compared to the hodo?
And, don't we have a trigger that does not depend on the hodoscopes, just the ecal?

For each clock cycle, all hodoscope hits in a range of time (before and after the clock cycle) are collected to build hodoscope patterns. Slides 8&9 in the attached file explain how the hodoscope pattern driver works. Updates of the Readout System for 2019 MC.pdf

@tongtongcao
Copy link
Contributor

tongtongcao commented Dec 6, 2024

One question: for those extra 5% triggers, could you check if GTP cluster and hodoscope pattern with 4ns shift come from the same signal, or by accident, one is from signal, while the other is from background, or even both of them are from background.
Another question: Is count of triggers for single3 or for single2&single3? In other words, is the steering file you used for multiple single triggers, or only for single3? As I remember (need to double check), single2 doe not require mapping between GTP and hodoscope, it means that even if no hodoscope matching with GTP, trigger will not missed since single2 trigger is still sent out.

@tongtongcao
Copy link
Contributor

tongtongcao commented Dec 6, 2024

You claimed that "the ecal gtp cluster that would trigger a 4ns tick earlier than the trigger gets the hodoscope patterns."
According to current setup for hodoscope, persistentTime = 60 ns, timeEarlierThanEcal = 0. It means that at a time clock (t), hodoscope hits between [t -60, t+4] will be collected to build hodo patterns.
https://github.com/JeffersonLab/hps-java/blob/master/ecal-readout-sim/src/main/java/org/hps/readout/hodoscope/HodoscopePatternReadoutDriver.java#L156-L164
How could be GTP cluster earlier than hodo patterns, where hodo hits persist 60ns?

@bloodyyugo
Copy link
Contributor Author

I am running both singles2 and singles3 triggers. I'm running this steering file (which I think is standard):

https://github.com/JeffersonLab/hps-java/blob/master/steering-files/src/main/resources/org/hps/steering/readout/PhysicsRun2019TrigSinglesWithPulserDataMerging.lcsim

It seems like the code on line 151 of the singles readout driver requires both pattern and gtp clusters be present for the trigger decision to be made:

https://github.com/JeffersonLab/hps-java/blob/054ee5d43d5e36618d248d41a6b8c124d2be3304/ecal-readout-sim/src/main/java/org/hps/readout/trigger2019/SinglesTrigger2019ReadoutDriver.java#L145C9-L155C27

So, even if it's looking for a singles2 trigger (no hodo in decision) the code does require that a pattern is present. As I mentioned in an above comment, this seems incorrect.

One question: for those extra 5% triggers, could you check if GTP cluster and hodoscope pattern with 4ns shift come from the same signal, or by accident, one is from signal, while the other is from background, or even both of them are from background. Another question: Is count of triggers for single3 or for single2&single3? In other words, is the steering file you used for multiple single triggers, or only for single3? As I remember (need to double check), single2 doe not require mapping between GTP and hodoscope, it means that even if no hodoscope matching with GTP, trigger will not missed since single2 trigger is still sent out.

@tongtongcao
Copy link
Contributor

Ah... I got where the issue took place:
https://github.com/JeffersonLab/hps-java/blob/054ee5d43d5e36618d248d41a6b8c124d2be3304/ecal-readout-sim/src/main/java/org/hps/readout/trigger2019/SinglesTrigger2019ReadoutDriver.java#L145C9-L155C27
It requires that size for both GTP clusters and hodo patterns are not 0 for any single trigger. Codes here are only for single3 trigger (we only needed events with single3 trigger before).
I missed to modify these codes when updating readout system to processing multiple triggers.
The bug causes issue when we process both single2 and single3 triggers, as you found.
The bug also significantly affects 2021 Moller MC since 2021 Moller MC readout uses this driver with single1 trigger.
Thanks for the nice finding. I will fix the bug with a PR soon.

…match spaced MC for both ecal and svt; change recon raw hit fitter driver to useTimestamps for mc; add some plots to KFOutputDriver
@bloodyyugo
Copy link
Contributor Author

@bloodyyugo
Copy link
Contributor Author

@mgignac
Copy link
Contributor

mgignac commented Jan 8, 2025

What's the status of this pull-request? From the above discussion, it looks to me that things started to converge. Is additional testing needed before this can go in and be used for the v6 MC production?

@sarahgaiser
Copy link
Collaborator

sarahgaiser commented Jan 8, 2025

I'm currently testing if the setup works for me. Readout worked well but I am having some trouble with reconstruction at the moment.
EDIT: the trouble I was having was just that slurm ran out of memory when running the reconstruction

@sarahgaiser
Copy link
Collaborator

sarahgaiser commented Jan 8, 2025

I am not sure if this is expected with this new method, but I see a bit of a spread in the cluster time using the no-spacing pipeline which isn't there for the spaced tritrig MC.
cluster_time_no_spacing_MC.pdf
cluster_time_new_recon_driver_MC.pdf
This is potentially a problem since we want to apply preselection cuts on the track-cluster time and might lose some valid but weirdly timed events.

EDIT: If you zoom in on the main peak, the spaced and the no-spacing cluster times are mostly the same with a mean of ~24.
cluster_time_no_spacing_fit_MC.pdf

the ADC readout window. The old version can be converted by
selecting a readout window equal to (readoutLatency - 64).
-->
<readoutOffset>55</readoutOffset> <!-- Units of 4 ns clock-cycles. -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 'spaced' version of the steering file this value is 10 instead of the 55 here. you probably changed this on purpose, but could you coment again why this value is different?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bloodyyugo can you comment on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readout offsets are (somewhat) arbitrary and are going to be different depending on whether your ADC buffers are referenced to a global clock (as in the spaced setup) or are independent (unspaced).

The readoutOffset here was tuned so that the ADC samples that were read out were the same as if we ran it in spaced mode...so, if there were e.g. four pedestal samples before the pulse started in spaced, there are four samples unspaced as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

the ADC readout window. The old version can be converted by
selecting a readout window equal to (readoutLatency - 64).
-->
<readoutOffset>4</readoutOffset> <!-- Units of 4 ns clock-cycles. -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar question here, why 4 instead of 10 (or 55 as above)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same tuning as above

@sarahgaiser sarahgaiser requested a review from mgignac January 10, 2025 12:33
@tongtongcao
Copy link
Contributor

tongtongcao commented Jan 10, 2025

Matt, does no-spacing have the same output from readout as spacing?
Sarah, have you taken comparison between spacing and no-spacing? Any difference between them?

@sarahgaiser
Copy link
Collaborator

They have different track times so I need to adjust the preselection time cuts and such to do a proper comparison. I haven't gotten around to that yet. However, I think merging this is okay for now, we can adjust this later as needed.

@tongtongcao
Copy link
Contributor

tongtongcao commented Jan 13, 2025

I think that we should use old spacing version for 2021 MC production until tests show that reconstruction results are consistent between spacing and no-spacing.
I suggest to wait for more tests for merge of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up MC readout to work without spacing events
4 participants