pycbc
pycbc copied to clipboard
gwastro
Add DQ ranking statistic to coinc search workflow
This pull request is still work in progress as it depends on #3804 and includes commits that will be added as part of that PR.
This PR adds support for the new DQ ranking statistic to be used in the coinc search workflow. This required a number of changes to pycbc_make_coinc_search_workflow to support the new executables and workflow structure. However, this new code should be fully back-compatible.
@dethodav as we are close to merging #3804, please make sure this is appropriately rebased. At the moment it conflicts with master and appears to introduce many of the things already introduced in #3804.
Ok we need to get this merged in. I'm willing to do whatever needs to be done but not sure where to start. @dethodav @titodalcanton
The first thing to solve is that this branch has developed a conflict with master, so a careful rebase is needed.
The first thing to solve is that this branch has developed a conflict with master, so a careful rebase is needed.
The repo is under Derek's namespace. Would the best way to handle this be to clone the repo to my namespace so I can rebase it, and then open a new PR?
@maxtrevor you may actually be able to push to Derek's branch. Give it a try, if it does not work you can create a new branch, though I would prefer to continue to use this PR for cleanliness.
@titodalcanton @maxtrevor I can take a look at this again - apologies for letting this sit for so long
@titodalcanton @maxtrevor I've gone ahead and rebased to the current master branch and implement some of the previous suggestions by @titodalcanton. I'm currently rerunning a full workflow to confirm that everything is still working after the rebase.
I just left a bunch of minor style suggestions. As a more general comment, the two plotting scripts added by this PR look very similar, does it make sense to consolidate them into a single script?
@dethodav was also running one more test to check everything is ok, I would like to hear from that before approving.
@titodalcanton My rerun of the workflow after rebasing was successful:
https://ldas-jobs.ligo.caltech.edu/~derek.davis/cbc/preO4/pycbc_idq/merge_request_tests/dq-workflow-simple-gwosc-chunkgwosc-1/
I did identify an issue where one of the plotting scripts can fail when small amounts of data are analyzed. I fixed this separately in this PR: https://github.com/gwastro/pycbc/pull/4153
I'll work on addressing these comments and other style suggestions as soon as I can.
Most of my comments above can simply be accepted by clicking the "commit suggestion" button, so should be fairly quick to address.
@dethodav any progress on this? We really need to get this merged in ASAP
I took the liberty of applying most of my suggestions, as they were really trivial. I think a rebase on master is needed now to make the CI tests pass.
@titodalcanton @maxtrevor I've finally updated they PR - let me know if there's any other changes I should make!