Eccentricity and mean anomaly support in bilby #861
Conversation
raffienficiaud
changed the title
raffienficiaud
force-pushed
the
bilby_v5ehm
branch
from
834bf43 to
b36df31
Compare
bilby/gw/conversion.py
Outdated
| @@ -259,7 +259,7 @@ def convert_to_lal_binary_black_hole_parameters(parameters): | |||
| ) | |||
| converted_parameters[f"cos_tilt_{idx}"] = 1.0 | |||
|
|
|||
| for key in ["phi_jl", "phi_12"]: | |||
| for key in ["phi_jl", "phi_12", "eccentricity", "mean_per_ano"]: | |||
There was a problem hiding this comment.
Thanks for this PR! My personal preference would be to not have the eccentricity and mean_per_ano fields in quasicircular PE files, so I would vote for not including these here at all. But I'd also like to hear opinions from others.
There was a problem hiding this comment.
Naively, I would have thought that eccentric PE with gwsignal_binary_black_hole would work even without changes to this line. Am I missing something?
There was a problem hiding this comment.
I introduced eccentricity and mean anomaly here because with the addition of eccentricity and mean_per_ano in the arguments of the gwsignal_binary_black_hole function in source.py, when one tries to run a quasicircular approximant (which does not specify eccentricity and mean_per_ano) Bilby would crash. Adding them here is a possible fix, but if you have other suggestions we are happy to modify the code
There was a problem hiding this comment.
Thanks for explaining. I still think there are other, better ways of dealing with this. Here are some in decreasing order of preference:
-
Refactor the current code by adding a
_base_gwsignal_cbc_fd_waveformfunction similar to_base_lal_cbc_fd_waveform, and makegwsignal_binary_black_holecall this function similar to howlal_binary_black_holecalled_base_lal_cbc_fd_waveform. Write another functiongwsignal_eccentric_binary_black_holewhich also calls_base_gwsignal_cbc_fd_waveform. This change will also make things easier if e.g. you want to add e.g. tides to one of the waveforms. -
If for some reason 1 isn't preferred, I think the easiest way to not make bilby crash is to fix
eccentricityandmean_per_anoto zero in your prior. -
You could also set a default value of
eccentricityandmean_per_anoin the function definition.
I think 1 is by far the most robust option. Tagging @asb5468 @ColmTalbot @mj-will for opinions.
There was a problem hiding this comment.
Thank you @adivijaykumar for the suggestions: is this addressing it? If yes, I'll continue from there and revert the other changes and tests.
Concerning 2/ and 3/, I would like to avoid default parameters as they may lead to silent mistakes.
There was a problem hiding this comment.
@raffienficiaud Yes I think that addresses it for the most part! You might want to take out the default waveform_kwargs from inside the _base_* function and put it into the individual gwsignal_* functions.
There was a problem hiding this comment.
Previously (#793) we had discussed not making further modifications to this function and any new desired functionality should be leveraged by directly using the new waveform generator as a subclass of bilby.gw.waveform_generator.WaveformGenerator as it will lead to much less of the spaghetti we currently have to deal with different specific cases.
There was a problem hiding this comment.
I reverted the changes in this file and the related unit test.
bilby/gw/conversion.py
Outdated
| @@ -259,7 +259,7 @@ def convert_to_lal_binary_black_hole_parameters(parameters): | |||
| ) | |||
| converted_parameters[f"cos_tilt_{idx}"] = 1.0 | |||
|
|
|||
| for key in ["phi_jl", "phi_12"]: | |||
| for key in ["phi_jl", "phi_12", "eccentricity", "mean_per_ano"]: | |||
There was a problem hiding this comment.
Previously (#793) we had discussed not making further modifications to this function and any new desired functionality should be leveraged by directly using the new waveform generator as a subclass of bilby.gw.waveform_generator.WaveformGenerator as it will lead to much less of the spaghetti we currently have to deal with different specific cases.
* add the new parameters * set new parameters to zero if not specified * unit tests
raffienficiaud
force-pushed
the
bilby_v5ehm
branch
from
b36df31 to
508dd5c
Compare
I am not sure to understand this statement @ColmTalbot : are you suggesting that this change is not required as gwsignal is supposed to handle it? There is a plugin infra coming into |
There is some more context in this issue: #793. FWIW, I agree with Colm. I really think we should avoid adding more waveforms via this approach if possible. |
|
@mj-will @ColmTalbot thank you for your comments. I know the work that has been done on The same issue as the one raised here affects LALSuite BTW, and we have to make intermediate steps until Until |
The PR adds support for the parameters
eccentricityandmean_per_anoanomaly.The changes have been tested for supporting the model SEOBNRv5EHM which review has recently been completed.
As this is my first contribution, let me know if you need more unit tests (I cannot run them all locally or in this fork).