gwastro / pycbc

Core package to analyze gravitational-wave data, find signals, and study their parameters. This package was used in the first direct detection of gravitational waves (GW150914), and is used in the ongoing analysis of LIGO/Virgo data.
http://pycbc.org
GNU General Public License v3.0
313 stars 347 forks source link

There's an off-by-one bug in pycbc_ifar_catalog OR the heirarchical removal code #3913

Closed spxiwh closed 2 years ago

spxiwh commented 2 years ago

There is a problem in pycbc_ifar_catalog if the number of signals in the data exceeds the "max_heirarchical_removals".

This basically can be seen by following a few lines. First here:

https://github.com/gwastro/pycbc/blob/master/bin/plotting/pycbc_ifar_catalog#L83

Where the number of removals is set. Lets say that this is 10.

Then here the code tries to access the "h10" entry:

https://github.com/gwastro/pycbc/blob/master/bin/plotting/pycbc_ifar_catalog#L122

However, with these settings the file will contain h0, h1 ... h9. It will not contain h10.

Now that I write this, it might be an off-by-one error in the heirarchical removal code itself if h9 means "I've done 9 removals". Perhaps it will always do one fewer removal than what was requested?

GarethCabournDavies commented 2 years ago

I think you're right on the hierarchical removal code. However I think it does do the removal, just doesn't write it to file :facepalm:.

The check/break for whether the max hierarchical level has been reached is on this line: https://github.com/gwastro/pycbc/blob/master/bin/all_sky_search/pycbc_add_statmap#L399

However the HR foreground and background haven't been written to the file at this point.

I think this should be solved by moving the max_hierarchical_removal check to the same place as the check/break for if the hierarchical removal ifar limit has been reached: https://github.com/gwastro/pycbc/blob/master/bin/all_sky_search/pycbc_add_statmap#L471

GarethCabournDavies commented 2 years ago

Solved by #3914