Remnant-fits.md

@@ -40,14 +40,17 @@ Initial comments
 
 * [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.
-* [ ] 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.
+    > 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.
 
 * [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py)
-* [ ] Documentation for functions is very inconsistent. For instance, while [`final_mass_of_merger`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L456) is very well documented [`final_mass_of_merger_from_NR`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L429) has no documentation.
-* [ ] [`final_params`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1298) should have a more specific name, e.g., `final_spin_params`, since it's just used for the final spin. Conversely, [`luminosity_params`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1307) should have a more general name, since it's also used for the final mass.
+* [x] Documentation for functions is very inconsistent. For instance, while [`final_mass_of_merger`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L456) is very well documented [`final_mass_of_merger_from_NR`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L429) has no documentation.
+* [x] [`final_params`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1298) should have a more specific name, e.g., `final_spin_params`, since it's just used for the final spin. Conversely, [`luminosity_params`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1307) should have a more general name, since it's also used for the final mass.
 * Minor issues:
 - [ ] Inconsistency in naming functions: [`m_total`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L106) versus [`mtotal`](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L113). There is also an appearance of `m_total` [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L143), but it looks like `mtotal` is the standard naming convention.
-- [ ] This is not really part of this review, but in [this code](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1190) using a calculation like [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L215) instead of `arccos` would presumably be faster, though perhaps less clean to read (and it's surely not a big performance issue). I would also recommend adding a comment about why things called `spin_magnitudes` in the code can be negative.
+- [x] This is not really part of this review, but in [this code](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L1190) using a calculation like [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L215) instead of `arccos` would presumably be faster, though perhaps less clean to read (and it's surely not a big performance issue). I would also recommend adding a comment about why things called `spin_magnitudes` in the code can be negative.
+    > I have kept `arccos` only because I think it is cleaner for readability and I do not think this will affect the performance of the code that much :) I have also changed `a_1` to be `spin_1z` which circumvents the need to explain why things called `spin_magnitudes` can be negative.
 
 ## Related MRs