Remnant-fits.md

@@ -52,6 +52,35 @@ Initial comments
 - [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.
 
+## Nathan Johnson-McDaniel 2020.03.08
+
+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)
+- [ ] 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))?
+- [ ] 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)
+- [ ] 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.
+- [ ] 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).
+
+* Minor:
+- [ ] ["fit you wish to you"](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L460); also [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L496)
+
+* For the future
+- [ ] This should only apply the BBH fits if there are no tidal deformability samples present (this will be necessary on the O3a catalogue timescale).
+- [ ] The `_final_from_initial()` function might also include `IMRPhenomD` as an option, interfacing with the Husa et al. fits used in this waveform.
+- [ ] Similarly, one might allow for the other EOB models known by [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py#L403) (see [here](https://git.ligo.org/lscsoft/lalsuite/-/blob/master/lalsimulation/lib/LALSimBlackHoleRingdown.c#L717) and [here](https://git.ligo.org/lscsoft/lalsuite/-/blob/master/lalsimulation/lib/LALSimBlackHoleRingdownPrec.c#L39))
+
+* [latex_labels.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/plots/latex_labels.py)
+* Minor:
+- [ ] There should be a space between ergs and s^{-1} [here](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/plots/latex_labels.py#L84). Additionally, things like "peak," "nonevol," "ergs," and "s" should all be set in Roman. 
+
 ## Related MRs
 
 * MR !242
\ No newline at end of file