iDQ based veto in PyCBC Live #4175
Conversation
Add support for dynesty's dynamic nested sampler (gwastro#3288)
Updating
Live combine fits missing bins bug (gwastro#3320)
Update branch
Updating fork
update repo
Update Master
Update master
Updating from gwastro
Rebase from gwastro
Rebase from gwastro master
bin/pycbc_live
Outdated
| # for triggers with valid idq info, only keep those with | ||
| # idq FAP > threshold | ||
| valid_idx = numpy.where(idq_valid)[0] | ||
| valid_keep = data_reader[ifo].idq.indices_of_flag( |
There was a problem hiding this comment.
I am a bit confused by what is being done here. What is the reasoning behind using at_times() for the iDQ status channel and indices_of_flag() for the iDQ FAP channel? Where are you actually checking that the iDQ FAP is above the given threshold?
There was a problem hiding this comment.
Sorry I overlooked the code that answers the second question, but the first one stands.
|
@maxtrevor overall comment: I think there is a genuine bug, apart from that this should do what we want. I would like to propose an alternative implementation though. I do not particularly like the idea of having to carry around two |
|
Ok I've addressed the obvious bug of using the wrong channel and the various one line changes you suggested. I will work on the logic for using both timeseries together and managing them with one object. |
|
In light of @maxtrevor's comment about iDQ status channel type, and after more carefully reviewing the current code in Note that the DataBuffer class has a dtype parameter. It defaults to float, but StatusBuffer changes it to integer when inheriting from DataBuffer. Given that iDQ channels are all float instead (let's ignore whether that is appropriate or not) then using two StatusBuffer objects to hold the iDQ data does not seem the correct approach to me. I think we really want a single iDQBuffer class that carries two float DataBuffer attributes for value and status, but does not inherit from DataBuffer itself. We are then free to implement whatever specific logic iDQ uses (based on floats or ints, one or two channels, etc) inside the iDQ-specific class, and StatusBuffer can continue to retain its current meaning as a simpler integer bitmask status information. Does this make sense? |
It does make sense. I think I am most of the way to having this working already. One thing occurred to me though. The StatusBuffer object uses dtype int unless told otherwise. So why was it giving me an error unless I explicitly cast to int? It seems like it used a float anyways even though the default dtype was int. This is mostly irrelevant but it doesnt seem to make any sense. |
|
@titodalcanton I think this now does everything we want |
|
This should be ready to go at this point, I have an analysis that has been running for the last 24 hours and has vetoes active at expected times |
|
This seems ok to be merged now. Just a minor final request: do you need to store the iDQ threshold as an attribute of the StrainBuffer? If not, please remove it. |
|
…and same comment for the |
* Added veto based on iDQ false alarm probability * Bugfix to use correct idq state channel, and address some of Tito's comments * Changed iDQ buffer object to manage two DataBuffer objects * Fixed stylistic issues * Fixed frame read error * Addressed Tito's comments
* Added veto based on iDQ false alarm probability * Bugfix to use correct idq state channel, and address some of Tito's comments * Changed iDQ buffer object to manage two DataBuffer objects * Fixed stylistic issues * Fixed frame read error * Addressed Tito's comments
This PR adds the option to create vetoes by thresholding on a data quality timeseries.