-
Notifications
You must be signed in to change notification settings - Fork 354
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
Fix a couple of output/upload value issues in pycbc live #4953
Fix a couple of output/upload value issues in pycbc live #4953
Conversation
@GarethCabournDavies would it be possible for you to get single detector candidate info into the foreground section of the 8 second trigger files as well? or too much for this one MR? |
Probably too much for this MR (and avoiding scope-creep of this one). I'll add an issue to say that a lot of this (saving/upload of candidates) seems to be repeated imperfectly between coincs and singles, so trying to get them to use the same code path would be better |
Ok. Getting the likelihood into the xml is already an improvement, thanks! |
@@ -303,7 +303,8 @@ def check(self, trigs, data_reader): | |||
|
|||
# fill in a new candidate event | |||
candidate = { | |||
f'foreground/{self.ifo}/{k}': cutall_trigs[k][i] for k in trigs | |||
f'foreground/{self.ifo}/{k}': cut_trigs[k][sngl_idx][i] |
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'm a bit confused by the change to this line? Why do we need sngl_idx here now when we didn't before?
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.
cutall_trigs
already had the sngl_idx
included (line 285 above), but cut_trigs
doesnt have that
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.
Ah I missed the change from cutall_trigs
to cut_trigs
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.
This looks good to me, approving
@@ -303,7 +303,8 @@ def check(self, trigs, data_reader): | |||
|
|||
# fill in a new candidate event | |||
candidate = { | |||
f'foreground/{self.ifo}/{k}': cutall_trigs[k][i] for k in trigs | |||
f'foreground/{self.ifo}/{k}': cut_trigs[k][sngl_idx][i] |
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.
Ah I missed the change from cutall_trigs
to cut_trigs
Update the chisquared uploaded to gracedb to be reduced, and save the ranking statistic value
Standard information about the request
This is a bug fix
This change affects the live search
This change changes scientific output
This change: follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
Motivation / contents
We noticed that the chisquared for singles was not the reduced chisquared - I tracked it down to where I'd missed that I was using the same trigger set for statistic calculation as I was for the upload, which is not what is done for coincs, this is now set back to the un-modified set of triggers.
We also realised that the ranking statistic isnt being saved - I have added this to the gracedb upload/xml by utilising the "likelihood" value in the coinc row of the table.
Testing performed
Need to test this, but thought it best to get the PR in first