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

refactor get_time_delay function in rno-g detector #732

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

fschlueter
Copy link
Collaborator

This reduces code for the most common way of using the function cable_only=False use_store=True

Copy link
Collaborator

@shallmann shallmann left a comment

Choose a reason for hiding this comment

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

did you test that it gives consistent results? I added some comments.

NuRadioReco/detector/RNO_G/rnog_detector.py Outdated Show resolved Hide resolved
NuRadioReco/detector/RNO_G/rnog_detector.py Outdated Show resolved Hide resolved
NuRadioReco/detector/response.py Outdated Show resolved Hide resolved
@fschlueter
Copy link
Collaborator Author

did you test that it gives consistent results? I added some comments.

Nope :(

@fschlueter
Copy link
Collaborator Author

Maybe a more general question also to @cg-laser, @sjoerd-bouma and @philippwindischhofer : I do not like this implementation with the cable_only (using strings in the component name ...). Is there a use case where one is interested in only the cable/fiber delay and not the delay of the entire response chain? If that is not the case I will delete the entire option cable_only

@fschlueter
Copy link
Collaborator Author

okay I removed the function cable_only entirely. Now the function get_cable_delay calls get_time_delay and this returns the time delay of the entire chain possibly including amplifier and filter.

@philippwindischhofer
Copy link
Member

Hey @fschlueter sorry to be slow, just catching up.

I think for most (non-expert) use-cases, we don't need to look separately at the cable delay. The (relative) delay between two separate signal chains is what is most important, I think.

@fschlueter
Copy link
Collaborator Author

@shallmann do you want to approve?

@shallmann
Copy link
Collaborator

on it... just looking at the changes once more now

shallmann
shallmann previously approved these changes Dec 3, 2024
Copy link
Collaborator

@shallmann shallmann left a comment

Choose a reason for hiding this comment

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

Please check if the print in the main is intended. Otherwise good to go.

print(det.get_time_delay(13, 0))
print(det.get_time_delay(13, 0, cable_only=True))
# print(det.get_time_delay(13, 0, cable_only=False, use_stored=False))
print(det.get_time_delay(13, 0, cable_only=True, use_stored=False))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The optional argument now no longer exists, no?

NuRadioReco/detector/response.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@shallmann shallmann left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up!

@fschlueter fschlueter merged commit 6f1c4a8 into develop Dec 3, 2024
5 checks passed
@fschlueter fschlueter deleted the extend_rnog_detector branch December 5, 2024 11:05
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