Skip to content
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

add 2-ifo phase/time/amp consistency to coinc rate stat #2830

Merged
merged 16 commits into from
Aug 15, 2019

Conversation

tdent
Copy link
Contributor

@tdent tdent commented Jul 4, 2019

No description provided.

@tdent tdent requested a review from ahnitz July 4, 2019 18:30
@tdent tdent force-pushed the amaldi_stat branch 2 times, most recently from 8d04cf4 to 5efed14 Compare July 5, 2019 14:12
@tdent tdent force-pushed the amaldi_stat branch 5 times, most recently from f3fce5d to 9282f7b Compare July 28, 2019 12:26
@tdent tdent changed the title add H-L phase/time/amp consistency to coinc rate stat add 2-ifo phase/time/amp consistency to coinc rate stat Jul 28, 2019
@tdent tdent force-pushed the amaldi_stat branch 2 times, most recently from 4b39e7a to 72dcaa4 Compare July 28, 2019 19:19
@tdent
Copy link
Contributor Author

tdent commented Jul 29, 2019

@ahnitz this seems to work minimally in terms of not breaking previously working multi-ifo behaviour.

@tdent
Copy link
Contributor Author

tdent commented Jul 30, 2019

@GarethDaviesGW I'm not looking for a full review here, just to check changes that affect the coinc-noise-rate stat and the network-sensitivity stat (sigma etc.)

Copy link
Member

@ahnitz ahnitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I think this structure is fine. Some minor comments. Also, it would be good to do a quick check by hand of pycbc_coinc_findtrigs (the two-ifo code) to make sure it still runs.

bin/hdfcoinc/pycbc_multiifo_coinc_findtrigs Show resolved Hide resolved
bin/hdfcoinc/pycbc_multiifo_coinc_findtrigs Show resolved Hide resolved
pycbc/events/stat.py Outdated Show resolved Hide resolved
pycbc/events/stat.py Outdated Show resolved Hide resolved
@GarethCabournDavies
Copy link
Contributor

@GarethDaviesGW I'm not looking for a full review here, just to check changes that affect the coinc-noise-rate stat and the network-sensitivity stat (sigma etc.)

They look good to me, and the list-of-tuples thing makes sense as an alternative to the dictionary for singles info. When I have tested before with the 'only use pta if using HL' bit, it worked and it doesn't look like that's changed, but I guess this will be changed anyway once the HV/LV files are made

@tdent
Copy link
Contributor Author

tdent commented Aug 2, 2019

Codeclimate is only 'similar blocks of code' (which all refer to comment strings!!) or 'too many arguments' (for a method that was already present with the same number of arguments).
@ahnitz @spxiwh I think this is ready to be merged, has been tested in a full workflow

Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, given previous discussion and explanation of codeclimate complaints.

@tdent
Copy link
Contributor Author

tdent commented Aug 4, 2019

Note that the choice to use the HL signal hist for HLV coincs is not necessarily the best simple approximation, for instance one could evaluate all 3 histograms and take a minimum or geometric mean over them. HL only does not take advantage of the amplitude test between V and other ifos which can suppress the majority of background

@tdent tdent merged commit 91ce41c into gwastro:master Aug 15, 2019
lenona pushed a commit to lenona/pycbc that referenced this pull request Sep 14, 2020
* first cut at coinc rate stat with p/t/a

* stat: combine background rate with p/t/a for HL

* allow verbosity in stat calc

* add statmap PEP8

* stat formatting and 3ifo p/t/a

* plumbing to pass ifo information to p/t/a stat

* second attempt at feeding through ifo info

* more rewrites for multiifo p/t/a

* coinc findtrigs formatting

* multiifo findtrigs towards pta

* multiifo p/t/a, first testing version

* multiifo p/t/a bug fix and more messages

* multiifo p/t/a more fixes

* multiifo p/t/a bugfix

* initialization of stat classes

* put lower floor on stat value
OliverEdy pushed a commit to OliverEdy/pycbc that referenced this pull request Apr 3, 2023
* first cut at coinc rate stat with p/t/a

* stat: combine background rate with p/t/a for HL

* allow verbosity in stat calc

* add statmap PEP8

* stat formatting and 3ifo p/t/a

* plumbing to pass ifo information to p/t/a stat

* second attempt at feeding through ifo info

* more rewrites for multiifo p/t/a

* coinc findtrigs formatting

* multiifo findtrigs towards pta

* multiifo p/t/a, first testing version

* multiifo p/t/a bug fix and more messages

* multiifo p/t/a more fixes

* multiifo p/t/a bugfix

* initialization of stat classes

* put lower floor on stat value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants