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

Allow negative delays in trace_utilities.delay #768

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

fschlueter
Copy link
Collaborator

This is not finished yet and will break the tests. We have to take a few decisions here. Currently I am always cropping the traces after delaying to remove the now unphysical signals at the beginning or end. However, in case of a positive delay this will change the trace start time (as it did before). This is now calculated in function. If we want to keep that we have to change the code where this function is used. I also changed the calculation of the number of samples to crop. We might need to check if that calculation agrees now with calculations outside of this function. And finally, why do we have the argument delayed_samples I would remove it.

… cropping. Also change calculation of the number of samples to be cropped. The original code was wron when time_delay / sampling_frequency was a round number.
@fschlueter
Copy link
Collaborator Author

Oh this function is only used in one module (analogToDigitalConverter) which makes the gravity of our decisions much smaller. Why it is not used in base_trace.apply_time_shift? Both do the same but apply_time_shift does not corp anything (which I think might be a problem). Should we not corp it in apply_time_shift but than zero-pad again to avoid changing trace start time and number of samples? The same we could add to this function here!

@fschlueter
Copy link
Collaborator Author

@cg-laser or @sjoerd-bouma any thoughts?

@fschlueter
Copy link
Collaborator Author

Okay one unit test is still failing which I will investigate. However, apparently the new warning I added is firing a lot during our simulation examples. Are we time-shifting traces with noise? Shouldn't we corp those traces than by default?

@fschlueter
Copy link
Collaborator Author

CPP version of ray tracer is available
fraction of triggered events = 114/10000 = 0.011 -> 8.07561 km^3 sr
effective volume deviates 2119.7 sigma (3%) from the mean
deviation is more than 3 sigma -> this should only happen in less than 1\% of the tests. Rerun the test and see if the error persists.

what is going on here?

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.

1 participant