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

Simulate trigger channels for RNO-G #743

Closed
wants to merge 66 commits into from

Conversation

fschlueter
Copy link
Collaborator

This PR is based on #716 and #741.

Currently the "everything regarding the trigger channels happens in the triggerBoardResponse module and the trigger module. I will also add a script which shows how to simulate the trigger.

@fschlueter fschlueter changed the base branch from develop to rnog_trigger November 15, 2024 14:42
@fschlueter
Copy link
Collaborator Author

@cg-laser that highlights the idea I have - feel free to comment. Currently everything happens in the triggerBoardResponse.py but in principle we could also do it in the HardwareResponseIncoperator. The one downside with how it is implemented right now is that I first fold in the radiant response and a cable and later fold them out which is somewhat awkward.

@sjoerd-bouma
Copy link
Collaborator

I'll try to have a look at this later this week but just so we don't forget - if we go with this implementation the trigger channel should probably be serialized as well, right? Right now I don't think it would be stored if you write to a .nur file.

@fschlueter
Copy link
Collaborator Author

I'll try to have a look at this later this week but just so we don't forget - if we go with this implementation the trigger channel should probably be serialized as well, right? Right now I don't think it would be stored if you write to a .nur file.

good point. this is missing yet. I will add it today!

@fschlueter fschlueter changed the base branch from rnog_trigger to develop November 19, 2024 10:27
@fschlueter
Copy link
Collaborator Author

@cg-laser that highlights the idea I have - feel free to comment. Currently everything happens in the triggerBoardResponse.py but in principle we could also do it in the HardwareResponseIncoperator. The one downside with how it is implemented right now is that I first fold in the radiant response and a cable and later fold them out which is somewhat awkward.

My main critique here is that this is not only awkward but could make things impossible as bandpass filters can't be inverted. As the bandpass for the FLOWER is narrower than for the RADIANT would work for the current RNO-G,but for other systems it might break.

This is now resolved. I am now creating and applying trigger responses in the hardwareResponseIncorperator.

@fschlueter
Copy link
Collaborator Author

Besides the issue #770 this PR is almost done. We can modify more trigger modules to use the trigger channel getters. So feel free to comment.

@fschlueter
Copy link
Collaborator Author

@cg-laser, Sjoerd and I just had a zoom call to discuss this issue. A short summary:

  • The unit tests are failing because now we add the cable delay after we added noise instead of before. That means that even if after adding the cable delay the signals are at the exact same sample/position in time as before, they are different relative to the noise we added. This explains the failing unit tests.
  • We agreed that we still want this change although it might be difficult to verify that everything is okay (because of the failing tests) for a couple of reasons. As already discuss is it easier when adding trigger channels, it is conceptually more clean, and if we start adding coherent noise (for example galactic noise) adding the cable delay before we add noise is simply wrong.
  • The way forward would be to double check my code and verify that the failing tests/differences in the results are within the tolerance we expect when changing noise.

Any thoughts comments?

@fschlueter
Copy link
Collaborator Author

Just thinking out loud here. Do we need to keep, in simulations, all trace_start_times the same? This clearly simplifies the trigger simulation. Of course we could account for different start times in the trigger simulations but it might be annoying to do that (in particular if we have added noise). I do not see a reason why this would be impossible, but is there one I do not see?

@fschlueter
Copy link
Collaborator Author

I will close this PR for the moment. The main functionality will be merged with PR #788. I will add a issue for the missing feature which was attempted in 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.

3 participants