| ... | ... | @@ -107,7 +107,7 @@ Initial comments |
|
|
|
- Inspected version: ae8d9f2d on the `review_comments` branch.
|
|
|
|
|
|
|
|
* [nrutils.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py)
|
|
|
|
* [ ] [`_bbh_final_spin_precessing_using_non_precessing_fit`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L217) sets `phi_12 = 0`, instead of passing it through.
|
|
|
|
* [x] [`_bbh_final_spin_precessing_using_non_precessing_fit`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L217) sets `phi_12 = 0`, instead of passing it through.
|
|
|
|
> I think this is now fixed. The code only sets `phi_12=0` when you are using the non-precessing fits for a non-precessing waveform. When you are using the non-precessing fits for a precessing waveform, the final_spin is corrected to take into account `phi_12` in the _bbh_final_spin_precessing_projected function. See commit: d69b4f72
|
|
|
|
|
|
|
|
* [x] I think that it's confusing to have a function named [`bbh_final_spin_precessing_Panetal`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L221) for a nonspinning fit.
|
| ... | ... | @@ -127,7 +127,7 @@ Comments on the revised version |
|
|
|
- Inspected version: ce5d729e on the `review_comments` branch
|
|
|
|
|
|
|
|
* [nrutils.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py)
|
|
|
|
- [ ] The [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L55) for `bbh_final_spin_non_precessing_Healyetal` (and the similar one for the final mass) should make it clear that this is an interface for the general `Healyetal` fits in nrutils, but can pass 2016 as the default version to get the Healy and Lousto fit by default.
|
|
|
|
- [x] The [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L55) for `bbh_final_spin_non_precessing_Healyetal` (and the similar one for the final mass) should make it clear that this is an interface for the general `Healyetal` fits in nrutils, but can pass 2016 as the default version to get the Healy and Lousto fit by default.
|
|
|
|
- [x] Is there any reason to keep the (nonspinning) Pan et al. fit in `FinalSpinPrecessingFits` ([here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L479))?
|
|
|
|
> No reason at all. I have removed it.
|
|
|
|
- [x] The `_bbh_final_spin_precessing_projected()` function's [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L279) needs to mention the augmentation with the in-plane spins
|
| ... | ... | |
