janosch314 / GWFish

Simulation of detector networks with Fisher-matrix PE
33 stars 20 forks source link

Improve waveform classes & derivatives numerical precision #84

Closed AlessandroAgapito23 closed 1 month ago

jacopok commented 6 months ago

Thanks for this @AlessandroAgapito23 ! I think the changes are overall good, but there are some rough edges to take care of. See my comments above. Also, you can see that the automated tests are failing - some of them were using GWFish's TaylorF2, and something is going wrong there with the spin variables. You can see the details from the Github interface here, or you can rerun the tests locally (see development installation).

If you want (not compulsory for this to be merged), it would also be great to have some tests for the new PPE waveforms. These could go into the test/test_waveforms.py file, or another one as you prefer. For example, a good and simple one might be to check that the PPE waveforms reduce to the regular TaylorF2 when the PE corrections are zero. You can see what the basic syntax of the tests looks like by looking at the ones that are already present; also you can check out the pytest docs.

Finally, since the waveforms module was originally @bvgoncharov 's child I think we could benefit from his opinion as well.

bvgoncharov commented 5 months ago

Hi @AlessandroAgapito23 , @jacopok , I reviewed the code, please see my comments above

AlessandroAgapito23 commented 5 months ago

Hi @bvgoncharov, unfortunately I don't see any comments above. Where can I find them?

AlessandroAgapito23 commented 5 months ago

Between line 507 and 590, so regarding "calculate_frequency_domain_strain() function", I changed the following things :

Note : I was wrong in writing the psi_tot in "TaylorF2", there was a typo in the self.psi definition. Now I think that the "TaylorF2" waveform should work, I'm going to check it.