tedwards2412 / ripple

Differentiable Gravitational Waveforms with JAX
51 stars 14 forks source link

TaylorF2 and IMRPhenomD_NRTidalv2 #19

Closed ThibeauWouters closed 2 months ago

ThibeauWouters commented 2 months ago

This PR adds the TaylorF2 and IMRPhenomD_NRTidalv2 waveforms in ripple.

As I already mentioned, it would be good to discuss whether the current approach with the utils_tidal.py file in the waveforms directory is a good idea for future work that will integrate the NRTidalv2 corrections on top of other BBH waveforms.

I am happy to discuss further and make changes accordingly!

tedwards2412 commented 2 months ago

This all looks great @ThibeauWouters! Couple of things to look at before merging:

ThibeauWouters commented 2 months ago

Hi @tedwards2412,

Thank you so much for the feedback! I totally agree with the .csv files, I think I was so used to them being there for testing the waveform that I just forgot to remove them basically.

I have renamed the tidal utils file, but the TaylorF2 waveform also uses one function from that file, which can be a bit confusing at this point. Depending on which waveforms will be added in the future, there might be some tidal utilities shared between all tidal waveforms, and some utilities shared between IMRPhenom waveforms with tidal effects, so perhaps we might have to change this again. I think apart from that, the renaming makes sense indeed.

Cheers

tedwards2412 commented 2 months ago

This looks great, thanks!