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

[pycbc live] Allowing the use of psd variation in the ranking statistic for pycbc live #4533

Merged
merged 33 commits into from
Nov 10, 2023

Conversation

ArthurTolley
Copy link
Contributor

Changes made to pycbc_live:

  • new argument for enabling psd_variation
  • calling the variation.py module to perform the psd_var calculation
  • creating filters
  • saving psd_var values to results

changes made to run.sh:

  • adding the psd_var argument and the new ranking statistic

changes made to matchedfilter.py:

  • adding a psd_var values array to store the values calculated

changes made to variation.py:

  • adding new functions for pycbc live specific psd_variation
  • create_filter
    • creates a filter for each ifo using the current best psd
  • calc_psd_variation
    • performs the convolution of the filter and the data to find the psd variation timeseries
    • average over 0.25s strides within this psdvar timeseries to remove the effect of any short duration glitches
    • remove any outliers from this 0.25s stride array
    • average further to retrieve a psd_var value for every second
  • this follows the method used in the offline psd_var calculation
  • find_var_value
    • take in the trigger end time and interpolate between the 2 psd_var values found for the seconds either side of the trigger. This is the final psd_var value for the statistic

@ArthurTolley ArthurTolley self-assigned this Oct 16, 2023
@ArthurTolley
Copy link
Contributor Author

Currently set to a draft PR:

  • Have code comments to address
  • Rename functions to be more explicitly pycbc live related
  • Testing extensively all the changes

@ArthurTolley
Copy link
Contributor Author

I am now happy with opening this up to review to discuss the changes and hopefully get this swiftly merged!

@ArthurTolley ArthurTolley marked this pull request as ready for review October 18, 2023 14:22
bin/pycbc_live Outdated Show resolved Hide resolved
bin/pycbc_live Outdated Show resolved Hide resolved
@ArthurTolley ArthurTolley requested a review from tdent October 19, 2023 08:32
@tdent
Copy link
Contributor

tdent commented Oct 19, 2023

Not sure I can review all the aspects concerning psd var calculation and pycbc live internal workings .. can we have more reviewers for those?

@tdent
Copy link
Contributor

tdent commented Oct 19, 2023

Do we know why the 'run small search' test fails?

@tdent
Copy link
Contributor

tdent commented Oct 19, 2023

Codeclimate seems to be finding some nontrivial problems (undefined variable, unused variable) as well as some easily fixable (line too long) and ones that might require some thought (code duplication within psd variation module)

@titodalcanton
Copy link
Contributor

Yup I will get to @ArthurTolley's PRs in the next days.

bin/pycbc_live Outdated Show resolved Hide resolved
@ArthurTolley
Copy link
Contributor Author

@tdent I assigned you because I believe you worked on the original PSD variation for offline with Simone. I will be very thankful for review from Tito too.

Just made some changes to fix a lot of the codeclimate and to re-run the small search test, hopefully it will pass this time.

The code duplication in variation.py might just have to be a thing, the codes are similar enough to share specific functionality but different enough they require new functions for Live. This could be refactored but I didn't want to touch anything offline search related.

bin/pycbc_live Outdated Show resolved Hide resolved
@tdent
Copy link
Contributor

tdent commented Oct 31, 2023

@ArthurTolley If you have 15 minutes or so to do the most obvious refactoring, could you make a function out of the the 7-line-plus-long-comment block which creates 'full_filt', which is identical between online and offline psd var (even the comment is the same) ?

@ArthurTolley
Copy link
Contributor Author

@tdent I've moved filter creation to its own function, just going to test it now

@ArthurTolley
Copy link
Contributor Author

It's working fine on my local test, will wait for PR tests to finish

@tdent
Copy link
Contributor

tdent commented Oct 31, 2023

@ArthurTolley I was referring to a piece of code 7 lines long, that is currently identical between the offline and online paths, ie appears twice in this file, with basically identical comments (# Make the weighting filter - bandpass, which weight by f^-7/6, etc) and creates an object called full filt. Not the bandpass code which does not appear twice. I am asking for the most obvious refactoring (making a function to avoid copy-paste duplications) which should quiet some of the codeclimate complaints

@ArthurTolley
Copy link
Contributor Author

@tdent In the future could you tag the lines you want me to change? That way I can respond to the change you're requesting and there won't be any confusion or time-wasting. I dislike us simply tagging each other in separate comments and making this 90+ comment PR even longer. I will do the changes you have requested now.

@tdent
Copy link
Contributor

tdent commented Oct 31, 2023

TBH I thought it would be unambiguous, eg if you search existing code for full_filt.

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.

Looks OK to me, if tests etc. pass

@tdent
Copy link
Contributor

tdent commented Nov 2, 2023

Think this is ready to go, the remaining 'duplication' complaints are minor

@ArthurTolley
Copy link
Contributor Author

Thank you @tdent, I'm glad you figured out that code climate line continuation, it was doing my head in.

@titodalcanton Would you like one final look or are you okay with this being merged?

@ArthurTolley
Copy link
Contributor Author

ArthurTolley commented Nov 9, 2023

I will merge this PR at the end of the day (5pm UTC) if no further comments are made 😁

@ArthurTolley ArthurTolley merged commit 93b5f98 into gwastro:master Nov 10, 2023
maxtrevor pushed a commit to maxtrevor/pycbc that referenced this pull request Dec 11, 2023
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Dec 19, 2023
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
titodalcanton pushed a commit to titodalcanton/pycbc that referenced this pull request Jan 19, 2024
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
titodalcanton added a commit that referenced this pull request Feb 6, 2024
* Remove reference to relative_example in docs

* Use RTLD_GLOBAL for libgomp (#4353)

* Use RTLD_GLOBAL for libgomp

In conda-forge/pycbc-feedstock#74 it was suggested to use RTLD_GLOBAL for libgomp. Let's see if this works fine with the test suite (which should answer @josh-willis 's concerns).

* Move import of ctypes/gomp into __enter__

* Try this

* Revert "Revert "Allow SNR optimizer to use candidate point in initial array (#4393)""

This reverts commit 7be12f1.

We are now catching up with master, where the bug originally introduced
by #4393 is fixed properly, so here I am undoing the temporary fix.

* SNR optimisation options for pycbc_live (#4432)

* Moving the live optimizer option changes to my own branch

* Completing the snr optimization argument group

* updating pycbc_live

* re-adding bug fix

* removing TODO message

* Bug with d_e options

* Adding optimizer-seed

* fixing the d_e optimizer

* replacing run.sh code

* resolve merge conflict

* fixing run.sh

* cleaning up args_to_string func

* changing comment

* codeclimate fixes

* module docstring

* Update module docstring copyright

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* Add gareth

* removing argv

* argument changing

* removing duplicated arguments

* minor CC points

* remove bug introduced when making CC happier

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* Improvements to single-detector trigger fitting code for PyCBC Live (#4486)

* Cleanup

* Cleanup

* Refactor duration bin parsing code and add support for reading from bank

* Minor fix/cleanup to logging

* Update CLI checks for duration bins

* Cleanup

* Ignore inconsistent config when combining

* Fix bug

* Fix typo

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* Comment from Gareth

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

* [pycbc live] Don't add snr options to command if they don't exist (#4518)

* Don't run snr optimizer setup if not optimizing snr

* moving the check to a more appropraite place

* setting snr_opt_options to None if not optimizing

* [pycbc live] Allowing the use of psd variation in the ranking statistic for pycbc live (#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* added scaling of initial pop in snr_optimizer (#4561)

* added scaling of initial pop

* init popn in optimize_di & pso func

* added changes in optimize_pso

* usig logging.debug for snr

* Do not set matplotlib's backend in internal modules (#4592)

* Set version to 2.1.4

* Remove reference to single_template_examples in docs

* Remove reference to hierarchical_model in docs

* Live: produce empty trigger fit plot for detectors with no triggers (#4600)

* Live: produce empty trigger fit plot for detectors with no triggers

* allow for below-threshold triggers

* fix thinko in option parsing for defaults (#4615)

* fix thinko in option parsing for defaults

When an option is not given at all getattr on the args object gives None, but we don't want to translate that into "--option-name None" on the command line.

* bugfix 

obviously we needed to define 'key_name' first ..

* Improvements to single fit plots (#4509)

* Improvements to single fit plots

* Apply suggestions from Gareth

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

---------

Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>

---------

Co-authored-by: Ian Harry <ian.harry@ligo.org>
Co-authored-by: Arthur Tolley <32394213+ArthurTolley@users.noreply.github.com>
Co-authored-by: Gareth S Cabourn Davies <gareth.cabourndavies@ligo.org>
Co-authored-by: Thomas Dent <thomas.dent@usc.es>
Co-authored-by: Praveen Kumar <86048588+PRAVEEN-mnl@users.noreply.github.com>
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
GarethCabournDavies pushed a commit to GarethCabournDavies/pycbc that referenced this pull request Jul 24, 2024
…ic for pycbc live (gwastro#4533)

* Modifying files to include psd variation in single detector statistic calculation

* ending variation.py with a blank line

* Changing to an increment agnostic solution

* removing change already fixed

* Updating function names and docstrings

* removing ToDos and adding more helpful comments

* Removing unused import

* Codeclimate fixes

* Removing excess logging and whitespace mistakes

* Removing unused objects + codeclimate fixes

* Updating comments and docstrings, removing matchedfilter changes

* Revert "Updating comments and docstrings, removing matchedfilter changes"

This reverts commit 0e6473a.

* Removing matchedfilter changes, updating comments and docstrings

* Move --verbose to the end of the commands

* more comment updates

* Repositioning filter recreation

* Changes to comments and removing whitespace

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* removing refchecks

* Adding option veification for psd variation

* Apply suggestions from code review

Co-authored-by: Thomas Dent <thomas.dent@usc.es>

* fixing EOL error

* Refactoring the filter creation function

* codeclimate fixes

* undo

* full_filt func

* removing indentation

* code climate

* code climate

* try to quiet codeclimate

* codeclimate doesn't know PEP8

* brackets obviate line continuation

---------

Co-authored-by: Thomas Dent <thomas.dent@usc.es>
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.

5 participants