| ... | ... | @@ -74,7 +74,7 @@ Maximum difference for e_rad_nonevol: 4.1033842990145786e-13 |
|
|
|
|
|
|
|
# Review comments
|
|
|
|
|
|
|
|
## David Keitel 20200304
|
|
|
|
## David Keitel 2020.03.04
|
|
|
|
|
|
|
|
* inspected version: ed801a91
|
|
|
|
* [nrutils wrappers](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/nrutils.py):
|
| ... | ... | @@ -148,6 +148,18 @@ Comments on the revised version |
|
|
|
* 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.
|
|
|
|
|
|
|
|
## David Keitel 2020.03.04
|
|
|
|
* From Nathan:
|
|
|
|
> The _final_from_initial() function might also include IMRPhenomD as an option, interfacing with the Husa et al. fits used in this waveform.
|
|
|
|
|
|
|
|
--> Let's revisit that function as a 4th step after adding spin evolution and surrogates, for producing the IMRPhenomPv3HM/SEOBNRv4PHM results in the 190521 implications paper.
|
|
|
|
* [ ] I'd prefer the names switched around, e.g. non_evolved_final_mass_source -> final_mass_source_non_evolved which would put the important part first
|
|
|
|
--> done in f4ed12dd01c5d33e66e5ce2430bc49b0db317e9a
|
|
|
|
* [ ] in _final_spin_of_merger you do a manual projection in the non_precessing case. Couldn't you just call _bbh_final_spin_precessing_using_non_precessing_fit here?
|
|
|
|
* [ ] Also I'm somewhat confused now when/where the explicit fit arguments are passed, and when not. From the main generate_all_posterior_samples, it calls e.g. _final_spin_of_merger, which then calls final_spin_of_merger, which then has defaults method="NR", approximant="SEOBNRv4", NRfit="average". So it currently always uses those defaults, is that right? Any other options would be a future MR?
|
|
|
|
--> answer from Charlie: "Yes so it currently uses those defaults. And yes, for interests of time, I will save other options for a future MR."
|
|
|
|
|
|
|
|
## Related MRs
|
|
|
|
|
|
|
|
* MR !242 |
|
|
\ No newline at end of file |
|
|
|
* !241
|
|
|
|
* !242 |
|
|
\ No newline at end of file |
