FEAT: add precessing spin transformation - #1104
Conversation
GregoryAshton
left a comment
GregoryAshton
left a comment
There was a problem hiding this comment.
Broadly looks good to me - albeit the checks are failing
7a82a82 to
6b7f838
Compare
1c0807a to
34fcfc3
Compare
adivijaykumar
left a comment
adivijaykumar
left a comment
There was a problem hiding this comment.
I like this change! I am worried about regressions that it introduces though. Quick checks run on my laptop show that this could make up ~10% of a single phenomPv2 call, as opposed to <1% before. It might be an okay tradeoff in anticipation of future developments. Any thoughts @ColmTalbot?
Also, I was a bit surprised to find the function in geometry.py. I have always assumed that it pertains to detector geometry and location. Not totally opposed to this being in geometry.py, but I still think it fits better in conversion.py.
| try: | ||
| iota, spin_1x, spin_1y, spin_1z, spin_2x, spin_2y, spin_2z = bilby_to_lalsimulation_spins( | ||
| theta_jn=theta_jn, phi_jl=phi_jl, tilt_1=tilt_1, tilt_2=tilt_2, | ||
| phi_12=phi_12, a_1=a_1, a_2=a_2, mass_1=mass_1, mass_2=mass_2, | ||
| reference_frequency=reference_frequency, phase=phase) | ||
| except ZeroDivisionError: | ||
| if catch_waveform_errors: | ||
| return None | ||
| else: | ||
| raise |
There was a problem hiding this comment.
Would it be possible to abstract this out to a separate function? It is repeated three times in the same file.
| "three_by_three_matrix_contraction", | ||
| "time_delay_geocentric", | ||
| "time_delay_from_geocenter", | ||
| "zenith_azimuth_to_theta_phi", |
There was a problem hiding this comment.
I think transform_precessing_spins should be added here.
|
@adivijaykumar, thanks! I'm happy to move it.
I hadn't thought to properly benchmark this, but it makes sense to. 
|
Updated tolerance for assert_allclose due to precision loss in different array backends.
Added error handling for ZeroDivisionError when calculating spins.
34fcfc3 to
e8dab28
Compare
|
Thanks @ColmTalbot! This is encouraging. I had performed the test by making waveform calls over a bunch of parameters, but I agree that one of the tutorials is probably the best to benchmark this. I also found really massive improvements with |
3 participants
This is a replacement for #1044 that includes the backend changes rather than attempting to rebase.