Remnant-fits.md

@@ -5,3 +5,15 @@ To calculate the non-evolved remnant fits, we call the functions in `lalinferenc
 The conversions are handled in by the [`pesummary.gw.file.conversions` module](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/conversions.py) and specifically the [`pesummary.gw.file.conversion._Conversion` class](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/conversions.py#L534).
 
 # Review comments
+
+## David Keitel 20200304
+
+*  inspected version: ed801a91
+*  [nrutils wrappers](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/nrutils.py):
+* [ ]  I only find the two functions that use `args[0]` etc a bit dangerous. (`_bbh_final_spin_precessing_using_non_precessing_fit` and `_bbh_final_spin_precessing_projected`)
+However, continuing to read in [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/conversions.py), it seems passing things as positional arguments is the overall design, and can't be easily fixed without global redesign, so this is ok.
+* [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/conversions.py)
+* [ ]  `final_mass_of_merger_from_NR` and `final_mass_of_merger` take `final_spin` arguments (presumably for the Healy et al fits), but don't pass them on. Since these extra arguments are only used in nrutils for minor speedup, this is probably not a problem, but it looks odd; maybe just drop them from your local interface?
+* [ ]  Why do `final_mass_of_merger_from_NR` and `final_spin_of_merger_from_NR`, when not in the "average" case, always call the respective `non_precessing` functions?
+* [ ] `_final_mass_of_merger` seems to be missing the tilt arguments...?
+* [ ] If those arguments to `_final_mass_of_merger` are changed, then probably in L1254f the param lists checking needs to be updated.
\ No newline at end of file