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.

Sign up for GitHub

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

Jump to bottom

Fix cases where ifar limit was not being applied #4574

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Fix cases where ifar limit was not being applied #4574

merged 5 commits into from
Dec 4, 2023

Conversation

GarethCabournDavies
Copy link
Contributor

There are some places where the FAR limit was not being applied. I'll try to find if there are any more, but these are the ones which we noticed

@GarethCabournDavies GarethCabournDavies added bug offline search v23_release_branch PRs applied to the v2.3.X release branch or to be cherry-picked if merging to master labels Nov 30, 2023
@GarethCabournDavies GarethCabournDavies self-assigned this Nov 30, 2023
@GarethCabournDavies
Copy link
Contributor Author

This looks to be all of them, by doing a grep for 'get_far' and checking that all outputs get the limits applied

@tdent
Copy link
Contributor

tdent commented Nov 30, 2023

Changes look OK, what level of testing do we want?

@GarethCabournDavies
Copy link
Contributor Author

Changes look OK, what level of testing do we want?

Up to you - I've rerun the case we found, and confirmed that the IFAR is limited properly.

Injections take a different code path to zerolag, so can't really use those to check this kind of oversight (and they are exclusive-only).

The only suggestion I have for large-scale testing is to rerun postprocessing on a chunk and ensure there are no singles with IFAR above the limit, or set different limits for different coinc types, and check them each

@GarethCabournDavies
Copy link
Contributor Author

Found one more place through the extensive testing I mentioned above - there may be another but I am not 100% sure

@tdent
Copy link
Contributor

tdent commented Dec 2, 2023

Hopefully you can get this pinned down then..

@GarethCabournDavies
Copy link
Contributor Author

Pinned it down to a (unrelated) bug which caused the IFO key for the significance dict to be in the wrong order. Fixed and final runthrough of testing now

@tdent
Copy link
Contributor

tdent commented Dec 4, 2023

Should I be seeing more commits or are you waiting for testing?

@GarethCabournDavies
Copy link
Contributor Author

Waiting for testing still

@GarethCabournDavies
Copy link
Contributor Author

Okay, all statmap files now have the limits applied in my testing

@GarethCabournDavies GarethCabournDavies merged commit 0d8ae9e into gwastro:master Dec 4, 2023
31 checks passed
@tdent
Copy link
Contributor

tdent commented Dec 4, 2023

@GarethCabournDavies can you explain more about the significance dict 'unrelated bug' ? Did it have any consequences for the current search outputs?

@GarethCabournDavies
Copy link
Contributor Author

@GarethCabournDavies can you explain more about the significance dict 'unrelated bug' ? Did it have any consequences for the current search outputs?

Basically, the IFOs were in the wrong order when supplied as arguments to the code. So when checking for a 'L1H1' key, there wasn't one, as it would be 'H1L1' in all other codes

This has no consequences to results unless applying a significance limit to multi-detector events (which we don't do anyway).

@GarethCabournDavies GarethCabournDavies deleted the far_limit_add_statmap_bugfix branch December 4, 2023 13:25
maxtrevor pushed a commit to maxtrevor/pycbc that referenced this pull request Dec 11, 2023
@GarethCabournDavies @maxtrevor
Fix cases where ifar limit was not being applied (gwastro#4574)
92336d5 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Dec 19, 2023
@GarethCabournDavies @bhooshan-gadre
Fix cases where ifar limit was not being applied (gwastro#4574)
b0adfdd 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
spxiwh pushed a commit to spxiwh/pycbc that referenced this pull request Jan 8, 2024
@GarethCabournDavies @spxiwh
Fix cases where ifar limit was not being applied (gwastro#4574)
732233a 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
spxiwh added a commit that referenced this pull request Jan 9, 2024
@spxiwh @GarethCabournDavies @WuShichao
Prepare v2.3.3 release (#4596)
132e9da 
* Use gating windows in sngl_minifollowup and allow 'triggers within vetoes' option (#4549)

* Use gating windows in sngl_minifollowup - also allow 'triggers within vetoes' option

* Minor fixes

* add infor if fewer triggers than requested

* Allow pycbc_sngls_minifollowup to use single ranking statistic

* Fix bug in rank_stat_single for quadsum/phasetd statistics

* mask may be boolean now

* fix broken pycbc_page_snglinfo due to bugfix

* page_snglinfo to use the ranking statistic used

* missed change

* Fix cases where ifar limit was not being applied (#4574)

* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict

* remove removal of downranked triggers (#4579)

* remove removal of downranked triggers

* add brief explanation into the caption

* bug in choosing the far calculation method (#4593)

* bug in which IFO combo is used for calculating the FAR in each combination during HR

* Safety catch

* Update release number

* add coordinates_space.py (#4289)

* add coordinates_space.py

* add LISA/SSB frame params

* add LISA_to_GEO and GEO_to_LISA

* add coordinates_space into FieldArray

* add doc and Astropy support

* update comments on sympy

* use fsolve from scipy instead

* fix cc issues

* fix cc issues

* minor fix

* update

* not use iteration

* decouple LISA orbit and more accurate Earth

* rename

* remove jplephem

* add the angular displacement of the Earth

* use radians

* make func readable in .ini

* reverse back to master

* correct psi range

* reverse to master

* fix unit issue in earth_position_SSB

* put LISA to the "right" position

* add LISA specific transform classes here

* change names

* update

* make a package for coordinates

* remove coordinates_space import

* move __all__ into __init__.py

* remove all coordinates_space

* change TIME_OFFSET to seconds

* fix SOBHB issue

* rename

* add SSB or LISA params into fid_params

* rename

* fix cc issues

* fix cc issue

* fix cc issue

* update

* update

* fix

* add default names

* overwrite params with same names

* remove pre-fixed names

* remove all pre-fixed names

* not pop

* fix inverse transform

* update tc

* not overwrite

* add SNR support for multi-model

* Update waveform.py

* t0 issue

* t0 issue

* Update space.py

* add obstime

* np.mod(psi_newframe, 2*np.pi)

* fix obstime

* add support for array inputs

* Update hierarchical.py

* just use Alex's implementation

* CustomTransformMultiOutputs is in another PR, so remove it

* add LDC and LAL convention correction

* use pycbc standard names

* more meaningful name

* use pycbc standard names

* Update relbin.py

* Update parameters.py

* remove unnecessary changes

* fix cc issue

* fix cc issue

* fix cc issue

* fix cc issue

* compactify

* compactify

* add __all__ back

* Update transforms.py

* Update transforms.py

* Update test_transforms.py

* Update transforms.py

* update doc

* fix time warning

* Update space.py

* Update test_transforms.py

* Create test_coordinates_space.py

* fix cc issues

* fix cc issues

* fix cc issue

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update test_coordinates_space.py

* add inline doc

* Update tox.ini

* add check of bbhx

* Update test_coordinates_space.py

* Update tox.ini

* Update test_coordinates_space.py

* add MultibandRelativeTimeDom into hierarchical.py

* Update __init__.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update __init__.py

* Update space.py

* Update space.py

* Update space.py

* fix psi issue

* Update test_coordinates_space.py

* Update test_coordinates_space.py

* update lalsimulation cvmfs path (#4580)

* update lalsimulation cvmfs path

* missed that the mount location needs to be changed as well

* more references to previsou CVMFS location

* Revert "add coordinates_space.py (#4289)"

This reverts commit e3418c7.

* REmoving lisa examples

* REmove inference examples

* Remove LISA deps

* Removing more LISA things

---------

Co-authored-by: Gareth S Cabourn Davies <[email protected]>
Co-authored-by: Shichao Wu <[email protected]>
@spxiwh spxiwh removed the v23_release_branch PRs applied to the v2.3.X release branch or to be cherry-picked if merging to master label Jan 16, 2024
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
@GarethCabournDavies @bhooshan-gadre
Fix cases where ifar limit was not being applied (gwastro#4574)
3d4ec19 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
@GarethCabournDavies @lpathak97
Fix cases where ifar limit was not being applied (gwastro#4574)
47d196d 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
@GarethCabournDavies @acorreia61201
Fix cases where ifar limit was not being applied (gwastro#4574)
54b6369 
* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict
GarethCabournDavies added a commit to GarethCabournDavies/pycbc that referenced this pull request Jul 24, 2024
@spxiwh @GarethCabournDavies @WuShichao
Prepare v2.3.3 release (gwastro#4596)
02a6055 
* Use gating windows in sngl_minifollowup and allow 'triggers within vetoes' option (gwastro#4549)

* Use gating windows in sngl_minifollowup - also allow 'triggers within vetoes' option

* Minor fixes

* add infor if fewer triggers than requested

* Allow pycbc_sngls_minifollowup to use single ranking statistic

* Fix bug in rank_stat_single for quadsum/phasetd statistics

* mask may be boolean now

* fix broken pycbc_page_snglinfo due to bugfix

* page_snglinfo to use the ranking statistic used

* missed change

* Fix cases where ifar limit was not being applied (gwastro#4574)

* Fix cases where ifar limit was not being applied

* some more missed cases / long lines

* redoing a couple more long lines

* missed one more place

* unrelated bug where the IFOs were not in the right order for use with the significance_dict

* remove removal of downranked triggers (gwastro#4579)

* remove removal of downranked triggers

* add brief explanation into the caption

* bug in choosing the far calculation method (gwastro#4593)

* bug in which IFO combo is used for calculating the FAR in each combination during HR

* Safety catch

* Update release number

* add coordinates_space.py (gwastro#4289)

* add coordinates_space.py

* add LISA/SSB frame params

* add LISA_to_GEO and GEO_to_LISA

* add coordinates_space into FieldArray

* add doc and Astropy support

* update comments on sympy

* use fsolve from scipy instead

* fix cc issues

* fix cc issues

* minor fix

* update

* not use iteration

* decouple LISA orbit and more accurate Earth

* rename

* remove jplephem

* add the angular displacement of the Earth

* use radians

* make func readable in .ini

* reverse back to master

* correct psi range

* reverse to master

* fix unit issue in earth_position_SSB

* put LISA to the "right" position

* add LISA specific transform classes here

* change names

* update

* make a package for coordinates

* remove coordinates_space import

* move __all__ into __init__.py

* remove all coordinates_space

* change TIME_OFFSET to seconds

* fix SOBHB issue

* rename

* add SSB or LISA params into fid_params

* rename

* fix cc issues

* fix cc issue

* fix cc issue

* update

* update

* fix

* add default names

* overwrite params with same names

* remove pre-fixed names

* remove all pre-fixed names

* not pop

* fix inverse transform

* update tc

* not overwrite

* add SNR support for multi-model

* Update waveform.py

* t0 issue

* t0 issue

* Update space.py

* add obstime

* np.mod(psi_newframe, 2*np.pi)

* fix obstime

* add support for array inputs

* Update hierarchical.py

* just use Alex's implementation

* CustomTransformMultiOutputs is in another PR, so remove it

* add LDC and LAL convention correction

* use pycbc standard names

* more meaningful name

* use pycbc standard names

* Update relbin.py

* Update parameters.py

* remove unnecessary changes

* fix cc issue

* fix cc issue

* fix cc issue

* fix cc issue

* compactify

* compactify

* add __all__ back

* Update transforms.py

* Update transforms.py

* Update test_transforms.py

* Update transforms.py

* update doc

* fix time warning

* Update space.py

* Update test_transforms.py

* Create test_coordinates_space.py

* fix cc issues

* fix cc issues

* fix cc issue

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update tox.ini

* Update test_coordinates_space.py

* add inline doc

* Update tox.ini

* add check of bbhx

* Update test_coordinates_space.py

* Update tox.ini

* Update test_coordinates_space.py

* add MultibandRelativeTimeDom into hierarchical.py

* Update __init__.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update hierarchical.py

* Update hierarchical.py

* Update relbin.py

* Update __init__.py

* Update space.py

* Update space.py

* Update space.py

* fix psi issue

* Update test_coordinates_space.py

* Update test_coordinates_space.py

* update lalsimulation cvmfs path (gwastro#4580)

* update lalsimulation cvmfs path

* missed that the mount location needs to be changed as well

* more references to previsou CVMFS location

* Revert "add coordinates_space.py (gwastro#4289)"

This reverts commit e3418c7.

* REmoving lisa examples

* REmove inference examples

* Remove LISA deps

* Removing more LISA things

---------

Co-authored-by: Gareth S Cabourn Davies <[email protected]>
Co-authored-by: Shichao Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdent tdent tdent approved these changes

Assignees

@GarethCabournDavies GarethCabournDavies

Labels
bug offline search
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GarethCabournDavies @tdent @spxiwh