Skip to content

Conversation

@GilbertRdzP
Copy link

To fix some issues with an offset in hit time information. Trigger Time and Trigger Shift were taken into consideration while defining hit time.

@nickwp
Copy link
Collaborator

nickwp commented Jul 15, 2025

I do not think this change is correct. The hit times in the npz files are supposed to be the same as the hit times in the WCSim root files. The trigger times are stored separately, the same as in WCSim root files, they should not be added to the hit times here.

Often we want to use the hit times relative to the trigger time, not the hit time added to the trigger time, so we keep them separate in the npz file. When using the npz files, you can choose to add the trigger times to the hit times if you want.

@nickwp
Copy link
Collaborator

nickwp commented Jul 15, 2025

Sorry I've just realised that it's not the trigger time you're using to modify the hit times, but the trigger info. Either way it's still not correct to do that. The hit times should remain unmodified.

I'm not actually sure what the TriggerInfo contains - I've never used it. The actual trigger time is usally available from the GetDate() of the header of the trigger, and this is stored in the npz file separately. If you need to store the TriggerInfo, then add it as a new variable, but don't modify the hit times.

@kmtsui
Copy link
Contributor

kmtsui commented Jul 16, 2025

The problem is mainly due to the constant trigger offset of 950 ns which is added to the hit time when NHit trigger is used in WCSim. This causes the hit time to be out of selection in e.g. here if you assume the usual selection range.
Probably a better way to do is to save this trigger offset in npz, and use this information to correct the time window during h5 conversion.

@nickwp
Copy link
Collaborator

nickwp commented Jul 16, 2025

We handle this in WatChMaL by removing any trigger offset later. That's what we have done for IWCD, which had the same offset, for years without any trouble. That max hit time parameter is also configurable, and should be set to a large number generally. The limit of 100 was only introduced for more recent simulations (e.g. for WCTE) that did not have the 950 offset any more. Either way, the trigger time (which should allow you to account for the 950 offset) is already saved in the npz file. It would be nice to add the TriggerInfo too.

@kmtsui
Copy link
Contributor

kmtsui commented Jul 16, 2025

yep if this is the usual practice then it's better to configure the max hit time parameter directly in h5 conversion. @GilbertRdzP do you understand the discussion?
I think it would be worth to save the trigger offset in npz anyway, as it can be used for comparison with truth track time.

@GilbertRdzP
Copy link
Author

Yes, I understand. I'll start working on saving just the Trigger Offset and not modifying the hit time information

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.

3 participants