| ... | @@ -101,13 +101,11 @@ Comments on the revised version |
... | @@ -101,13 +101,11 @@ Comments on the revised version |
|
|
- Inspected version: ce5d729e on the `review_comments` branch
|
|
- Inspected version: ce5d729e on the `review_comments` branch
|
|
|
|
|
|
|
|
* [nrutils.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py)
|
|
* [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] 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))?
|
|
- [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.
|
|
> 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
|
|
- [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
|
|
|
|
|
|
|
|
* Possibly for the future
|
|
|
|
|
- [ ] I would recommend changing the `Healyetal` fit name (e.g., [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L53)) to something more descriptive, likely involving the year, since there is also a Healy, Lousto, and Zlochower fit, and more than one Healy and Lousto fit (and possibly more such fits in the future). However, the naming can probably stay as is for the moment, unless David has strong feelings about this.
|
|
|
|
|
|
|
|
|
|
* [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py)
|
|
* [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py)
|
|
|
- [x] The check `elif "pan" in NRfit.lower()` [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L470) and elsewhere is not future-proof, since one could imagine another fit name having "pan" inside it. Checking for "panetal" would be much safer.
|
|
- [x] The check `elif "pan" in NRfit.lower()` [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L470) and elsewhere is not future-proof, since one could imagine another fit name having "pan" inside it. Checking for "panetal" would be much safer.
|
|
|
- [x] The `_final_from_initial()` function's [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L395) should make it clear that in the precessing case it doesn't compute the final mass and spin the same way as in the waveform models (i.e., using the EOB fit evolution).
|
|
- [x] The `_final_from_initial()` function's [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L395) should make it clear that in the precessing case it doesn't compute the final mass and spin the same way as in the waveform models (i.e., using the EOB fit evolution).
|
| ... | | ... | |
