| ... | ... | @@ -92,90 +92,7 @@ max l_peak difference: 0.0 |
|
|
|
```
|
|
|
|
## Review comments
|
|
|
|
|
|
|
|
### David Keitel 2020.03.04
|
|
|
|
|
|
|
|
* inspected version: ed801a91
|
|
|
|
* [nrutils wrappers](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/nrutils.py):
|
|
|
|
* [x] 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.
|
|
|
|
> I have changed the code to make the arguments explicit rather than args[0]. This does not solve the problem in its entirety as this would require, as you say, a global redesign. See commit: f2c694a9
|
|
|
|
|
|
|
|
* [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/conversions.py)
|
|
|
|
* [x] `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?
|
|
|
|
> `final_spin` is now passed to the Healy et al fits if it has been calculated. See commit: ae8d9f2d
|
|
|
|
* [x] 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?
|
|
|
|
> For the `final_mass_of_merger_from_NR`, the only available fits are `non_precessing` so that is why it only called `non_precessing` fits. I have corrected the `final_spin_of_merger_from_NR` to allow for precessing fits. This is done by prepending the `NRfit` by `precessing_`. See commit: 7c57bdd2
|
|
|
|
* [x] `_final_mass_of_merger` seems to be missing the tilt arguments...?
|
|
|
|
> Tilt angles are not required for PESummary because `spin_1z` and `spin_2z` are already precomputed and can be passed directly.
|
|
|
|
* [x] If those arguments to `_final_mass_of_merger` are changed, then probably in L1254f the param lists checking needs to be updated.
|
|
|
|
> See previous comment.
|
|
|
|
* [x] There is this check `if all(i in self.parameters for i in final_params):` for calling `_final_spin_of_merger`. So what happens if the tilts are missing? No final spin computed at all? Shouldn't there be a non-precessing fallback?
|
|
|
|
> This has been corrected. See commit: cc5a6e59
|
|
|
|
|
|
|
|
* Higher level comments / looking ahead:
|
|
|
|
* [ ] How will the surrogates be called? Presumably something like `NRfit="NRSur7dq4"` instead of `NRfit="average"`? But it's not easy to see how this will be propagated up in the interface to user input.
|
|
|
|
> The main way that people will use this conversion function is through the command line tool `summarypages`. Therefore I think the best way would be to add a new command line argument `--NRSur-fit` which will use the NRSur fits.
|
|
|
|
* [ ] 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)
|
|
|
|
* [x] [`_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 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)
|
|
|
|
* [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.
|
|
|
|
- [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)
|
|
|
|
- [x] The [documentation](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/nrutils.py#L55) for `bbh_final_spin_non_precessing_Healyetal` (and the similar one for the final mass) should make it clear that this is an interface for the general `Healyetal` fits in nrutils, but can pass 2016 as the default version to get the Healy and Lousto fit by default.
|
|
|
|
- [x] 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))?
|
|
|
|
> No reason at all. I have removed it.
|
|
|
|
- [x] 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
|
|
|
|
|
|
|
|
* [conversions.py](https://git.ligo.org/lscsoft/pesummary/-/blob/review_comments/pesummary/gw/file/conversions.py)
|
|
|
|
- [x] 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.
|
|
|
|
- [x] 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 spin evolution).
|
|
|
|
|
|
|
|
* Minor:
|
|
|
|
- [x] ["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 the LALSimulation functions called by `_final_from_initial` [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.
|
|
|
|
|
|
|
|
### David Keitel 2020.03.09
|
|
|
|
* 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.
|
|
|
|
* [x] 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?
|
|
|
|
* [x] 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."
|
|
|
|
moved to [subpage](Remnant-fits-review-comments)
|
|
|
|
|
|
|
|
## sign-off
|
|
|
|
|
| ... | ... | |
