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

Ranking statistic for live singles #4689

Merged

Conversation

GarethCabournDavies
Copy link
Contributor

This PR does a couple of things:

Copy link
Contributor Author

@GarethCabournDavies GarethCabournDavies left a comment

Choose a reason for hiding this comment

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

Minor explanation comments

@@ -6,7 +6,7 @@
import h5py
import numpy as np

f = h5py.File('single_trigger_fits.hdf','w')
f = h5py.File('single_significance_fits.hdf','w')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renaming to clarify the two types of single fits

pycbc/events/single.py Show resolved Hide resolved
@@ -682,7 +682,7 @@ def rank_stat_single(self, single_info,
numpy.ndarray
The array of single detector statistics
"""
return single_info[1]
return single_info[1]['snglstat']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows the phasetd rank_stat_single to actually work!

@GarethCabournDavies
Copy link
Contributor Author

GarethCabournDavies commented Apr 11, 2024

BTW - I requested reviews from both Arthur and Tito as I am aware that Arthur has the expertise in adding the statistic objects to Live, and Tito understands more of the significance fitting, so I don't expect you both to be able to review all of it

examples/live/make_fit_coeffs.py Show resolved Hide resolved
@@ -158,43 +198,62 @@ def check(self, trigs, data_reader):
# Apply cuts to trigs before clustering
# Cut on snr so that triggers which could not reach newsnr
# threshold do not have newsnr calculated
if 'psd_var_val' in trigs:
Copy link
Contributor Author

@GarethCabournDavies GarethCabournDavies Apr 11, 2024

Choose a reason for hiding this comment

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

See here for the psdvar conversions

pycbc/events/single.py Show resolved Hide resolved
pycbc/events/stat.py Show resolved Hide resolved
Copy link
Contributor

@ArthurTolley ArthurTolley left a comment

Choose a reason for hiding this comment

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

Just a single question from me but happy to approve. Glad to see the fits being included in the live example too!

@@ -711,8 +711,8 @@ def coinc_lim_for_thresh(self, sngls_list, thresh, limifo,
if not self.has_hist:
self.get_hist()

lim_stat = [b['snglstat'] for a, b in sngls_list if a == limifo][0]
s1 = thresh ** 2. - lim_stat ** 2.
fixed_stat = [b['snglstat'] for a, b in sngls_list if a != limifo][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a change in logic (going from == to !=) or was this not working properly before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't working properly before - the statistic from the limifo was being used instead of the fixed IFO network

Looking again, I don't think this will actually work, so I will check it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was broken, just in a different way.

The basic problem was this - the sngls_list passed to coinc_lim_for_thresh did not contain the limifo singles (here). As a result, the list comprehension was empty, and the [0] at the end of the line broke.

The problem with my fix was that by using only the first entry, this works for 2-ifo, but not for 3-ifo (but doesn't actually error).

As the list of sngls passed to coinc_lim_for_thresh doesn't contain the limifo, we can remove this check, but it should be a sum of squares rather than grabbing the zeroth entry.

@GarethCabournDavies
Copy link
Contributor Author

Note: new commits do not affect the part of the code which Arthur has reviewed

@GarethCabournDavies
Copy link
Contributor Author

poke @titodalcanton to look at the supervision scripts here

Comment on lines 212 to 214
(trig_chisq <
self.thresholds['reduced_chisq']) & \
(trigs['snr'] >
(trig_snr >
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to effectively change the meaning of --reduced-chisq-threshold and --newsnr-threshold in case the PSD var statistic is used, i.e. "reduced chisq" and "newsnr" will no longer mean what people historically think they mean. Should we rename the two options then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd like to change these to use the template-cuts and trigger-cuts module eventually, but for now I have updated single-newsnr-threshold to be call single-ranking-threshold, and and added a note to the help on the chisq threshold

Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>
Copy link
Contributor

@titodalcanton titodalcanton left a comment

Choose a reason for hiding this comment

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

I think we can merge and start proper tests on this now, provided the CI passes.

@titodalcanton titodalcanton enabled auto-merge (squash) June 11, 2024 13:22
@titodalcanton titodalcanton merged commit d486ec2 into gwastro:master Jun 11, 2024
33 checks passed
GarethCabournDavies added a commit to GarethCabournDavies/pycbc that referenced this pull request Jun 24, 2024
* Allow the live single trigger fits to use ranking statistic rather than sngl-ranking

* inbin is no longer all the events above threshold, plotting to indicate with case where no triggers are found

* deal better with cases where there are no triggers

* Use ranking statistic for single-detector events

* Fix some errors

* fix some statistics so they can produce single-detector events

* Some codeclimate suggestions

* get fit coeff files into CI, set a maximum IFAR for singles

* alter the CI example run

* Codeclimate suggestions

* Line too long

* minor tweaks

* Used shared code

* Fix broken fixing

* missed that this needs the module

* typo

* calculate plotmax earlier and use it to decide the histogram bins

* Update bin/live/pycbc_live_plot_single_significance_fits

Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>

* TDC comments

* Update threshold naming and description

* update argument in example

* Please do not look at the previous commit and see how much of an idiot I am

* Update bin/live/pycbc_live_combine_single_significance_fits

Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>

---------

Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants