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

Make gate-and-inpainting starting time precise #4403

Merged
merged 25 commits into from
Sep 22, 2023

Conversation

yi-fan-wang
Copy link
Member

In this PR, I timeshift GW strains such that the gating starting time lands exactly on a specific data sample.

@yi-fan-wang
Copy link
Member Author

A visulization of waveform and data being shifted before and after can be found here:
shiftgating
The notebook to generated the above fig is here. The configuration file used is in the same folder.

@yi-fan-wang
Copy link
Member Author

I launched two runs to estimate the kerr 220 ringdown mode with GW150914, one with this PR applied and one without. The result is here. They look close enough

@yi-fan-wang yi-fan-wang changed the title Make gate-and-inpainting starting time precise [WIP] Make gate-and-inpainting starting time precise Jun 22, 2023
@cdcapano cdcapano self-assigned this Jun 22, 2023
Copy link
Contributor

@cdcapano cdcapano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. But can you put lines 563-575 into a separate function that can be called? Maybe make it a class method of BaseGatedGaussian. That will make it easier to implement in the MarginalizedPolarization model. So, for example, in the GatedGaussian you would pass rtilde to do the shift, while in the MarginalizedPolarization model you would pass hp, hc, and the data.

@yi-fan-wang
Copy link
Member Author

yi-fan-wang commented Jul 12, 2023

@cdcapano After a second thought, now I implement the "making the gating time precise" differently. All the inference's models are left untouched, the time shifting is only implemented in pycbc.types.timeseries when calling TimeSeries.gate().

What the codes do is basicaly firstly shifting data to an earlier time by an amount equal to gate_end_time - the closest (floor) discrete data sample's time, then gating and inpainting, then shifting the data back to the original time (by shifting the same amount of time but to the later direction). I also change the inpainting array's index from [lindex:rindex] to [lindex:rindex+1], this makes the right index is included.

I test this PR by running GW150914's ringdown part with only 220 mode. This plot is a comparison of results with and without this PR applied. I see small but visible differences.

@yi-fan-wang
Copy link
Member Author

Got a figure to visulize it. The second figure is a zooming-in
subgating-data
subgating-data-zoomin

Copy link
Contributor

@cdcapano cdcapano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more changes, as we discussed on the call. Looks good otherwise.

pycbc/strain/gate.py Outdated Show resolved Hide resolved
pycbc/strain/gate.py Outdated Show resolved Hide resolved
pycbc/types/timeseries.py Show resolved Hide resolved
@yi-fan-wang
Copy link
Member Author

@cdcapano all comments addressed. I tested with GW150914 with ringdown 220 (config file). This plot compares results with and without this PR. Without this PR, A_220 = 2.8E-21; while results with this PR reduce A_220 a little bit to 2.77E-21. I think this is expected, because a little bit more pre-ringdown data are gated by this PR, hence smaller A220.

@yi-fan-wang
Copy link
Member Author

@ahnitz Collin and I would like to let you review this PR before merging, because it affects gate and inpainting everywhere.

The idea is that the previous gating start and end time are not precise when it is located at a subsample time. Say the LIGO data are sampled at ..., t_-2, t_-1, t_0, t_1, ... and the gating end time is somewhere between t_0 and t_1, previous gate-and-inpainting would inpaint only until t_-1, sometime even t_-2 if the window length is not a integer times of 1/sampling rate. This PR effectively reconstruct the time domain data at epoch t_gate_end and t_gate_end plus/minus n times of 1/sampling rate, then inpaint until one element before the t_gate_end. The reconstruction is done by time shifting in frequency domain and then shifting back after inpainting. We think this makes the gating epoch more precise, this will affect the results of ringdown overtone analysis where the characteristic decay time is even less than 1 ms.

pycbc/types/timeseries.py Outdated Show resolved Hide resolved
pycbc/types/timeseries.py Show resolved Hide resolved
pycbc/types/timeseries.py Outdated Show resolved Hide resolved
@cdcapano
Copy link
Contributor

@yi-fan-wang It looks like you've addressed all the comments that @ahnitz and I had, is that correct? Do you think this is ready to merge?

@yi-fan-wang
Copy link
Member Author

@ahnitz @cdcapano All comments addressed. I have launched a few runs and confirm that the commits after Alex's comments has the same results as before. There is a "line too long" complaint from codeclimate (85 characters > 79). I don't know. I kind of think a single line has better readability. If you also think this is minor, then I'll leave as what it is.

(Plots to compare runs with commits after Alex's comments and before https://hypatia.aei.mpg.de/~yiwang/ringdown/atlas-result/srate2018-4th-result.png and https://hypatia.aei.mpg.de/~yiwang/ringdown/atlas-result/srate2018-5th-result.png. They look close enough)

Also note that (1) this PR changes gate and inpainting everywhere, and it doesn't have backward compatibility, so we should add a notice in GW190521 data release repository and anywhere relevant. (2) after the fork of this branch, there were many new updates for PyCBC inference workflow, for example, take care of #4453. Also, the most recent pegasus, 5.0.6 only works with the most updated (at the time of writing) PyCBC

@yi-fan-wang
Copy link
Member Author

@ahnitz @cdcapano if you don't have objections, this is ready to merge

@yi-fan-wang
Copy link
Member Author

In case the above hypatia link doesn't work I drop the two figures here. These are runs with ringdown mode 220. The two figures have different starting times. The label "old runs in atlas" is the result before Alex's comments, while "after PR" is after. They look close enough.
srate2018-4th-result
srate2018-5th-result

Copy link
Contributor

@cdcapano cdcapano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes, look fine to me.

@cdcapano cdcapano merged commit caac90b into gwastro:master Sep 22, 2023
31 of 33 checks passed
@yi-fan-wang yi-fan-wang deleted the gatingsub branch September 22, 2023 22:10
jakeb245 pushed a commit to jakeb245/pycbc that referenced this pull request Sep 28, 2023
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Oct 3, 2023
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
GarethCabournDavies pushed a commit to GarethCabournDavies/pycbc that referenced this pull request Oct 9, 2023
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
GarethCabournDavies pushed a commit to GarethCabournDavies/pycbc that referenced this pull request Oct 11, 2023
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
PRAVEEN-mnl pushed a commit to PRAVEEN-mnl/pycbc that referenced this pull request Nov 3, 2023
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* apply time shift to account for the gating start time is a subsampling data point. fix a few typos

* fix int type error

* correct a misunderstanding for apply_fd_time_shift

* correct a unused variable

* correct the shift time

* address cc issues

* apply static method

* complete the gated_gaussian_noise

* typo

* typo

* re express time shift

* space

* start to work on pol marg model

* complete shifting for marg pol model

* instead of modifying models, now implementing time shifting only in gate-and-inpaint session

* remove all changes in the model

* remove changes in model

* fix some bugs when fft/ifft converting, now productive

* remove rindex+1 in gate and inpainting, add if condition for time shifting

* get rindex_time via actually doing the math, this is faster than retrieve the data sample time array

* remove the proj and projslc attributes in gated_gaussian_model, it's used for debugging before
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.

3 participants