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

Avoid nan values in network volume term - MASTER VERSION #5050

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

GarethCabournDavies
Copy link
Contributor

When testing live with the new ranking statistic, we were getting a lot of weird FARs - this is due to getting stat values which were nan, and these were considered louder than everything else in the count_n_louder.

Standard information about the request

This is a bug fix
This change affects the live search, and touches code of the offline search, but shouldn't affect anything
This change changes scientific output
This change follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

Contents

We combat this problem in three ways, which should prevent it occurring as much as possible:

  1. If median sigma from fits files for any benchmark-network detector is NaN, then we ignore it. As a result, e.g. NaNs in L1 should not affect H1 singles, which was the case before. Hopefully this will cut most cases, as it goes from being an "or" to an "and" for H1 and L1
  2. If the benchmark is NaN - i.e. both detectors have NaNs in the fits files - then we do not apply the log sensitivity term.
  3. As a final fail-safe, in the significance calculation, move any background with statistic NaN to have statistic of -np.inf. This shouldn't happen due to the protections above

Downsides of this approach:

  • If there is a much-less-sensitive detector which has NaN values, then the down-ranking from this factor will not be as it applied
  • If there is a foreground event which has statistic NaN, then it would be ranked higher than all background, I don't think this should be possible given the protections 1 and 2 in place, but this is a concern about protection 3

Links to any issues or associated PRs

#5049

Testing performed

None yet

  • The author of this pull request confirms they will adhere to the code of conduct

Copy link
Contributor

@tdent tdent left a comment

Choose a reason for hiding this comment

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

Changes look OK based on the related Live branch PR.

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
@tdent
Copy link
Contributor

tdent commented Feb 20, 2025

Minor polishing on comments, but looks OK to me assuming tests pass.

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
@tdent
Copy link
Contributor

tdent commented Feb 20, 2025

We want np -> numpy in stat.py 👀

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.

2 participants