Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[pycbc live] Allowing the use of template fits in the pycbc live ranking statistic #4527
[pycbc live] Allowing the use of template fits in the pycbc live ranking statistic #4527
Changes from all commits
379da30
84f28a7
e2b157d
211eb61
0a39883
ffc484c
4c63d57
bdb8060
e6d264f
47c91bd
d6d020c
c95d19a
357742b
73a73c3
4add7a9
6715e2f
120f8d9
ceea096
7f6166f
5136b97
0e747d6
402a449
631dc22
21ce202
84c63b0
d00efc3
c367a6c
befd1c4
fa5abe5
861ce2b
640a68b
173835d
7a0a0b0
7169170
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot judge if this is correct, I do not understand what this bit is doing just by looking at it (read: this bit needs comments). Seems strange that the assertion is inside the except block though, is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share Tito's concerns.
I can't figure out what setup would ever cause the
if ifo is None
check to be True, and whether takingifo = self.ifos[0]
makes sense in this context. I think comments should explain what situation each part of this is meant to handle (i.e. online vs offline, singles vs coincs, etc.).I think the if statement and assert can move outside the except block without any change of functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I echo that this needs comments.
If trigs is dict-like you'll get to the except, then if
ifo
is not a key in the trigs object, then it could be met.I am fairly sure that
self.ifos[0]
is not the right thing to get here though, as that will always get the same ifo, even when you're using the triggers from a different detector.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GarethCabournDavies when do we expect trigs to be dict-like vs not? I've lost track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dict-like will be when it is called using:
pycbc_fit_sngls_by_template
pycbc_page_snglinfo
andpycbc_sngl_minifollowup
pycbc_fit_sngls_binned
(it shouldn't really do this, and should use SingleDetTriggers)Not dictlike will be when using ReadByTemplate, which is done in
pycbc_coinc_findtrigs
andpycbc_sngls_findtrigs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping there was some clean division like always dict-like when in PyCBC live and always not in offline, but I guess thats not the case. Might be nice to make that more consistent in the future, but not a task for this MR. From what I can see, I do think its the case that Live always uses the dict-like path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is only used in live in the
single_stat = self.stat_calculator.single(trigsc)
line above, andtrigsc
is dictlike, so it seems this is safe