Improvements to single-detector trigger fitting code for PyCBC Live #4486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Gareth S Cabourn Davies <[email protected]>
GarethCabournDavies
approved these changes
Sep 15, 2023
|
This is proving extremely hard to test given that running a fitting job on past data takes many hours. Any advice @GarethCabournDavies? |
That's surprising - it has (in my experience) taken about an hour to run a day's triggers, and then the combination is fairly quick. Most of the time is IO on the files If you look in |
|
Yes I think the issue is just in reading the files. I am trying to get a local sample of trigger files on a fast local partition to get around that. |
|
Anything stopping this being merged now? |
|
Still have not tested fully, but I am slowly progressing. |
|
Ok, I was able to put together a couple test scripts which run quickly over a couple days: https://ldas-jobs.ligo.caltech.edu/~tito.canton/pycbc/test_pr4486 I tested the current configuration with the old code, the current configuration after this PR, and @GarethCabournDavies what do you think? |
jakeb245
pushed a commit
to jakeb245/pycbc
that referenced
this pull request
Sep 28, 2023
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
bhooshan-gadre
pushed a commit
to bhooshan-gadre/pycbc
that referenced
this pull request
Oct 3, 2023
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
GarethCabournDavies
added a commit
to GarethCabournDavies/pycbc
that referenced
this pull request
Oct 9, 2023
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
GarethCabournDavies
added a commit
to GarethCabournDavies/pycbc
that referenced
this pull request
Oct 11, 2023
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
PRAVEEN-mnl
pushed a commit
to PRAVEEN-mnl/pycbc
that referenced
this pull request
Nov 3, 2023
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
titodalcanton
added a commit
to titodalcanton/pycbc
that referenced
this pull request
Jan 19, 2024
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
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 <[email protected]> * 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 <[email protected]> Co-authored-by: Thomas Dent <[email protected]> * 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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]> * [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 <[email protected]> * removing refchecks * Adding option veification for psd variation * Apply suggestions from code review Co-authored-by: Thomas Dent <[email protected]> * 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 <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]> --------- Co-authored-by: Ian Harry <[email protected]> Co-authored-by: Arthur Tolley <[email protected]> Co-authored-by: Gareth S Cabourn Davies <[email protected]> Co-authored-by: Thomas Dent <[email protected]> Co-authored-by: Praveen Kumar <[email protected]>
bhooshan-gadre
pushed a commit
to bhooshan-gadre/pycbc
that referenced
this pull request
Mar 4, 2024
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
acorreia61201
pushed a commit
to acorreia61201/pycbc
that referenced
this pull request
Apr 4, 2024
…wastro#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 <[email protected]> * Comment from Gareth --------- Co-authored-by: Gareth S Cabourn Davies <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a couple issues with the single-detector trigger fitting code used by PyCBC Live.
First of all, the current code requires a manual specification of the template duration bins. If the specified bins do not cover the entire duration range of the templates, the search will crash as soon as a trigger is produced from one of the "misrepresented" templates. This PR adds an option to read the durations directly from the bank and calculate the bins accordingly on the fly, so that we no longer have to manually keep track of the durations (we do still need to tell the code if we switch to a different bank file path, but that is hopefully harder to forget).
Second,
pycbc_live_combine_single_fitswill currently refuse to combine a set of fit files done with different fitting configurations (e.g. different duration bins). If we do decide to change the configuration at some point during the run, this is probably not the ideal behavior (apart from getting a bunch of errors for 20 days, the fit coefficients will be effectively frozen for the same period). With this change,pycbc_live_combine_single_fitswill instead restrict the combined fit parameters to just the files with the same configuration, assuming the "right" configuration is the one from the most recent input file. This seems a better behavior to me, but I am open to other opinions.I also did some invasive code cleanup (most of it in separate commits), hopefully not too hard to review.
I have not tested this yet.