| ... | @@ -12,8 +12,13 @@ The conversions are handled in by the [`pesummary.gw.file.conversions` module](h |
... | @@ -12,8 +12,13 @@ The conversions are handled in by the [`pesummary.gw.file.conversions` module](h |
|
|
* [nrutils wrappers](https://git.ligo.org/lscsoft/pesummary/-/blob/master/pesummary/gw/file/nrutils.py):
|
|
* [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`)
|
|
* [ ] 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.
|
|
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)
|
|
* [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?
|
|
* [ ] `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?
|
|
* [ ] 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...?
|
|
* [ ] `_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.
|
|
* [ ] If those arguments to `_final_mass_of_merger` are changed, then probably in L1254f the param lists checking needs to be updated.
|
|
|
|
|
|
|
|
* 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.
|
|
|
|
* [ ] 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? |
|
|
|
\ No newline at end of file |
