| ... | ... | @@ -32,6 +32,23 @@ However, continuing to read in [conversions.py](https://git.ligo.org/lscsoft/pes |
|
|
|
* [ ] Similar for the spin evolution, I see that various functions already have a `evolved=False` placeholder switch, but how will that actually be decided? Will `generate_all_posterior_samples` get an extra switch argument (or as part of `self`), or will the evolved samples be generated in a step prior to calling this function and it will check for their presence, then use them if available?
|
|
|
|
> I was thinking of the latter: `will the evolved samples be generated in a step prior to calling this function and it will check for their presence, then use them if available?`. I think this is the cleanest way of solving this problem.
|
|
|
|
|
|
|
|
## Nathan Johnson-McDaniel 2020.03.05
|
|
|
|
|
|
|
|
Initial comments
|
|
|
|
|
|
|
|
- Inspected version: ae8d9f2d on the `review_comments` branch.
|
|
|
|
|
|
|
|
* [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.
|
|
|
|
|
|
|
|
* [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.
|
|
|
|
* 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.
|
|
|
|
|
|
|
|
## Related MRs
|
|
|
|
|
|
|
|
* MR !242 |
|
|
\ No newline at end of file |
