[pycbc live] Allowing the use of template fits in the pycbc live ranking statistic

[ArthurTolley]

This PR contains the changes required in pycbc to allow the use of the trigger fit files in the pycbc live search. This does not contain the scripts necessary to create these trigger fits files.

Any comments and questions are very welcome and I can share the files I have created for testing these changes.

[ArthurTolley]

I'm going to get this branch to the state of passing the github tests (should be there now) and then run a small test to see if everything passes. This includes doing all the offline statmaps file creations because those are affected by the changes to the stat, I don't want to break any of the offline code

[ArthurTolley]

All of the changes made which reference the parameter mchirp or curr_mchirp are required for the ExpFitFgBgNormBBHStatistic statistic. I have moved the setting of self.curr_mchirp out of the other stat and into a new method rank_stat_coinc which explicitly sets this parameter and then runs the previous rank_stat_coinc from the non-BBH fits statistic. Thanks @GarethCabournDavies for helping me unpick that knot!

[titodalcanton]

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?

[maxtrevor]

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 taking ifo = 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.

[GarethCabournDavies]

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.

[maxtrevor]

@GarethCabournDavies when do we expect trigs to be dict-like vs not? I've lost track

[GarethCabournDavies]

Dict-like will be when it is called using:

• a dictionary keyed on the dataset names like pycbc_fit_sngls_by_template
• a SingleDetTriggers object, as in pycbc_page_snglinfo and pycbc_sngl_minifollowup
• the h5py file directly as in 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 and pycbc_sngls_findtrigs

[maxtrevor]

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?

[GarethCabournDavies]

I think it is only used in live in the single_stat = self.stat_calculator.single(trigsc) line above, and trigsc is dictlike, so it seems this is safe

[maxtrevor]

Adding some review comments, mostly responding to Tito's previous comments to try and help move this along.

[maxtrevor]

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 taking ifo = 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.

[GarethCabournDavies]

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.

[GarethCabournDavies]

I've just cherry-picked a commit from master which should allow checks to progress more than they have, maybe that will fix things

[GarethCabournDavies]

I think the CI test failure is unrelated, so I will approve, and see if anyone provides a fix over the weekend

[maxtrevor]

I see all the required checks passed, happy to merge this

[ArthurTolley]

Just wanted to say thank you to everyone on this commit for getting this over the line while I was on holiday. Nice to see it finally merged!