Compressed waveforms bank workflow #4969

GarethCabournDavies · 2024-12-04T16:20:11Z

In order to make the generation of the bank with compressed waveforms much easier, and making this more straightforward for future developers/users, I've put this into a workflow.

I've also made changes which mean that when running pycbc_compress_bank, we can exclude certain waveforms; e.g. SPAtmplt, and these will be ignored when making the compressed bank.

When fetching the compressed bank, the fact that the template hash key isn't present will then mean that the waveform is just generated as normal.

Standard information about the request

This is a new feature
This change affects the offline search
This change changes nothing for scientific output, just makes things easier
This change: has appropriate unit tests, follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines

This change will require additional dependencies for the CI: sbank
sbank will also need updated from its curent form as it needs a tweak i made to the hdf5 bank combiner there (I will link PR when I have made it)

Splits the bank
Adds compressed waveforms to each split bank
Recombines these banks into any number of banks
- Note that the offline search only uses compressed waveforms for the inspiral jobs, i.e. where the bank is split anyway.
Plot the waveform compression in histograms / scatter points and add to a results page

The plots are similar to:

the number of / types of these plots can be chosen in the config file

Links to any issues or associated PRs

sbank PR:

Testing performed

The workflow was run, and the results page can be seen at https://ldas-jobs.ligo.caltech.edu/~gareth.cabourndavies/testoutput/compress_bank_workflow/testing/compress_bank_results_page_new/

I will add more tests (e.g. profiling) soon

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

ahnitz · 2024-12-04T16:23:49Z

tools/install_travis.sh

@@ -50,5 +50,8 @@ pip install -r companion.txt
 echo -e ">> [`date`] installing mpi4py"


Are we using this script anymore?

Hmmm - that might be why Im having trouble getting the CI to work! I'll have a look once i am back from holiday

GarethCabournDavies · 2024-12-13T11:27:48Z

Note this needs gwastro/sbank#64

I'm not sure whether to temporarily point to my branch, or to wait for a release

spxiwh

One change requested.

spxiwh · 2025-01-15T17:09:15Z

pycbc/waveform/bank.py

+
+        try :
+            if not (self.has_compressed_waveforms and self.enable_compressed_waveforms):
+                raise ValueError


I'm a bit concerned that we could have a situation where get_decompressed_waveform isn't working, but we don't know because we're always falling back on the slower method.

I've added a UserWarning so that if this happens, at least there is evidence, and the message of which includes the approximant as well, so that it is immediately obvious if something is wrong

(Note that as the message changes, it will not be supressed for different approximants when repeated, see https://docs.python.org/3/library/warnings.html#repeated-warning-suppression-criteria)

I meant that we should distinguish between self.has_compressed_waveforms being false, where we should silently fall back, and a call to self.get_decompressed_waveform( failing, which would indicate a genuine problem.

(Not clear why self.get_decompressed_waveform failures would not be ValueErrors???)

Ah, that makes sense. I've moved to a combined if/else with try/except which doesn't rely on the get_decompressed_waveform function not raising a ValueError

spxiwh · 2025-01-15T17:11:03Z

pycbc/workflow/core.py

@@ -1122,12 +1122,13 @@ def __init__(self, ifos, exe_name, segs, file_url=None,
        exe_name: string
            A short description of the executable description, tagging
            only the program that ran this job.
-        segs : igwn_segments.segment or igwn_segments.segmentlist
+        segs : igwn_segments.segment or igwn_segments.segmentlist or None


This change is rejected (or at least musn't be bundled in with this patch). Following other workflows, we provide a "dummy" segment if there is not a logical one to give, but a segment should still be given.

This has been reverted - it means that the files in the workflow are all named "...-0-0.ext" or similar, but that is a minor thing

spxiwh

One change requested.

spxiwh

I'm still a bit confused about the fallback logic around calling the compressed waveforms, but will approve at this stage anyway. I don't need to see this again before it's merged if you choose to make any further changes to that logic.

GarethCabournDavies · 2025-01-20T16:24:04Z

CI failure looks to be a Segfault in the 3-IFO coinc jobs - I will double-check that the inspiral output is the same before and after this change before I merge

GarethCabournDavies added the offline search label Dec 4, 2024

ahnitz reviewed Dec 4, 2024

View reviewed changes

GarethCabournDavies force-pushed the compress_bank_workflow branch from 41647e3 to 9989f4d Compare December 12, 2024 15:07

GarethCabournDavies force-pushed the compress_bank_workflow branch 2 times, most recently from a725f05 to 867d6ff Compare December 16, 2024 16:37

GarethCabournDavies added 16 commits January 15, 2025 08:03

Add compressed waveforms to bank workflow

b6e48f6

Allolw plotting script to use any bank conversion parameter

5903a1c

Some fixes to allow the joined bank to be plotted

e2fa601

Use inference's parameter labels: they are available and mostly good

43df615

Add mismatch to plotting, make some tweaks

84689d2

some tidying

4b15401

thinko

e048c44

Try to make the CI workflow run

4781eee

Fix do-not-compress default

064038f

Use different examples in compress bank workflow

0bf5908

Proper name for the github workflow

168cb98

Thinko

639bd2c

python shebang in compression workflow script

58be1dd

minor edits

d6cc8f0

move to readily-available waveform

38cf2b8

TRy IMRPhenomD instead

f23706f

GarethCabournDavies force-pushed the compress_bank_workflow branch from 867d6ff to f23706f Compare January 15, 2025 16:03

GarethCabournDavies requested review from spxiwh and ahnitz January 15, 2025 16:55

spxiwh requested changes Jan 15, 2025

View reviewed changes

GarethCabournDavies added 2 commits January 16, 2025 04:06

revert change to workflow.core

fb5bbc3

Warn for KeyError in get_decompressed_waveform

1d3ac81

GarethCabournDavies requested a review from spxiwh January 16, 2025 14:10

GarethCabournDavies changed the title ~~Draft: Compressed waveforms bank workflow~~ Compressed waveforms bank workflow Jan 17, 2025

spxiwh approved these changes Jan 17, 2025

View reviewed changes

Fix issue with if get_decompressed_waveform raised a ValueError

f7acd9e

GarethCabournDavies merged commit 91db678 into gwastro:master Jan 22, 2025
30 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compressed waveforms bank workflow #4969

Compressed waveforms bank workflow #4969

GarethCabournDavies commented Dec 4, 2024

ahnitz Dec 4, 2024

GarethCabournDavies Dec 4, 2024

GarethCabournDavies commented Dec 13, 2024

spxiwh left a comment

spxiwh Jan 15, 2025

GarethCabournDavies Jan 16, 2025

GarethCabournDavies Jan 16, 2025

spxiwh Jan 17, 2025

spxiwh Jan 17, 2025

GarethCabournDavies Jan 20, 2025

spxiwh Jan 15, 2025

GarethCabournDavies Jan 16, 2025

spxiwh left a comment

spxiwh left a comment

GarethCabournDavies commented Jan 20, 2025

		@@ -50,5 +50,8 @@ pip install -r companion.txt
		echo -e ">> [`date`] installing mpi4py"

Compressed waveforms bank workflow #4969

Compressed waveforms bank workflow #4969

Conversation

GarethCabournDavies commented Dec 4, 2024

Standard information about the request

Contents

Minor changes to compression

Making a workflow to generated the compressed waveform bank(s)

Links to any issues or associated PRs

Testing performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarethCabournDavies commented Dec 13, 2024

spxiwh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxiwh left a comment

Choose a reason for hiding this comment

spxiwh left a comment

Choose a reason for hiding this comment

GarethCabournDavies commented Jan 20, 2025