Closed bvgoncharov closed 1 year ago
Let me say that the things I'm mentioning are minor and overall I think this is a great addition!
I agree that these are good changes, but I don't think that all of Jacopo's comments are minor. For example, the way the roll-off is done here might not be safe, and in fact, this is one of the issues we found with time-domain waveforms in the past, which can have important impact on PE. This needs to be tested carefully on signals with varying masses.
Thanks for reviewing the code, @janosch314 and @jacopok ! As for the roll_off
argument in the fft
function, the function is not actually used anywhere at the moment, and I will remove it to avoid any confusion. The data is conditioned in LAL, so I do not foresee any problems with tapering.
Ok, I think I addressed all the comments, @jacopok and @janosch314 . Unless there are any objections from you and also @u-dupletsa , I could merge this PR within a few days.
As a bonus, I now added citation information to README.md
, you can see how it looks here: https://github.com/bvgoncharov/GWFish . I think it looks good, but let me know if you would like to change/remove it.
Hi Boris,
Thank you for all the work!
I’m going through the changes in the code and I don’t know if this is important or not, but I have a question about the reference frequency in the Waveform class. Before we were setting it to 50Hz (with this choice, if I remember well, the LAL TaylorF2 and the GWFish TaylorF2 were almost identical) and now it is set by default to the minimum frequency. I recall that setting f_ref to a low frequency was creating some problems. Is there a reason behind the choice to choose the minimum frequency?
Ulyana
On 8 Feb 2023, at 12:00, Boris Goncharov @.***> wrote:
Ok, I think I addressed all the comments, @jacopok https://github.com/jacopok and @janosch314 https://github.com/janosch314 . Unless there are any objections from you and also @u-dupletsa https://github.com/u-dupletsa , I could merge this PR within a few days.
As a bonus, I now added citation information to README.md, you can see how it looks here: https://github.com/bvgoncharov/GWFish https://github.com/bvgoncharov/GWFish . I think it looks good, but let me know if you would like to change/remove it.
— Reply to this email directly, view it on GitHub https://github.com/janosch314/GWFish/pull/55#issuecomment-1422411515, or unsubscribe https://github.com/notifications/unsubscribe-auth/ATUPTCDWM7C2DWC2AGNKA7LWWN4EVANCNFSM6AAAAAAUS72SWY. You are receiving this because you were mentioned.
Hi Ulyana, thanks for looking into the changes! Good point identifying the change in reference frequency, I forgot about it. I introduced it when debugging time-domain waveforms. I considered the following when setting f_ref = f_low
:
f_ref = f_low
.f_ref = f_low
, which I expected could introduce less trouble when FFT'ing time-domain waveforms. Empirically, I ran a quick test now, and I see more consistency between mass parameter errors in PhenomXPHM and PhenomTPHM when setting f_ref = f_low
, compared to setting e.g. f_ref = 50 # Hz
.f_ref
by hand :) Hi, sorry for the extra N corrections, I hadn't properly looked through the waveforms
module before.
I think the t_of_f is the most important one potentially - at the moment, all waveforms are computed in January 1980!
The warning spam is a bit annoying but not a big deal, I hope.
Hi All,
I went through the code again, put back the default reference frequency of 50 Hz in CBC_Simulation.py
following a discussion with @u-dupletsa , addressed @jacopok 's comments. Thanks to both of you for the review!
I also removed function hphc_caller
because there are many useful internal parameters in waveform objects that would be useful to access in the main script, e.g. CBC_Simulation.py
. Loading waveforms through hphc_caller
only provided access to the end result. And implementing new waveform would require also changing hphc_caller
, which is extra work. Instead, a user can now directly choose a waveform class, and load the waveform through it. To achieve this, I also made GWFish-intrinsic waveforms TaylorF2
and PhenomD
as classes. The goal was only to make the code functional, so I did not change anything within these waveforms, e.g. I did not break them into different methods, etc.
As usual, if there are no more comments, I could do a merge in a few days. Tagging @janosch314 , FYI.
Looks good! As written at the moment it breaks the horizon
module (since it's importing hphc_amplitudes
), but I think this PR can be merged as-is and I can update it to the new syntax quickly thereafter.
@jacopok , thank you, that was not too hard and I fixed it. Also, for CBC_Background.py
, docs/source/figures/SNR_function_of_distance.py
, tests/test_waveforms.py
. In case we find something else, we can create ad-hoc pull requests. It looks like the main issues are resolved, so I am merging this PR!
I added the following modifications:
modules/waveforms.py
. To facilitate this, functionlal_caller()
is replaced with classes. There is a new basic waveform class,Waveform(object)
. Time-domain LAL waveforms are either Fourier-transformed into frequency domain in LALSimulation (class LALFD_Waveform(Waveform)
) or returned in time-domain and Fourier transformed in GWFish (class LALTD_Waveform(LALFD_Waveform)
) before calculating the Fisher matrix.modules/fishermatrix.py
, derivative and Fisher matrix functions are also replaced by classes.There are also a few minor cosmetic changes.
Notes: