bilby issueshttps://git.ligo.org/lscsoft/bilby/-/issues2021-04-28T16:57:19Zhttps://git.ligo.org/lscsoft/bilby/-/issues/569Add caching of the `generate_posterior_sample_from_marginalized_likelihood` m...2021-04-28T16:57:19ZGregory Ashtongregory.ashton@ligo.orgAdd caching of the `generate_posterior_sample_from_marginalized_likelihood` methodIf interrupted, this has to start from scratch which can cause endless loops.If interrupted, this has to start from scratch which can cause endless loops.Gregory Ashtongregory.ashton@ligo.orgGregory Ashtongregory.ashton@ligo.orghttps://git.ligo.org/lscsoft/bilby/-/issues/559check_draw doesn't catch nan log_likelihood values.2021-03-18T05:26:20ZEamonn O'Sheacheck_draw doesn't catch nan log_likelihood values.I've been getting dynesty errors from nan loglikelihoods during construction of the Dynesty sampler object. I was stepping through the bilby internals with pdb and was finding that the nan loglikelihoods were happening in `get_initial_points_from_prior` in the `base_sampler.py`. I was confused as to why the function `check_draw` wasn't preventing `theta`'s which produce nan loglikelihoods from being added to the returned list. I believe the reason is a bug in `check_draw`, which has the following implementation.
```
def check_draw(self, theta, warning=True):
bad_values = [np.inf, np.nan_to_num(np.inf), np.nan]
if abs(self.log_prior(theta)) in bad_values:
if warning:
logger.warning('Prior draw {} has inf prior'.format(theta))
return False
if abs(self.log_likelihood(theta)) in bad_values:
if warning:
logger.warning('Prior draw {} has inf likelihood'.format(theta))
return False
return True
```
However, nan's don't behave as other values with respect to equality. `nan != nan` is `True`, so if `self.log_likelihood(theta)` is nan, then the check becomes `abs(np.nan) in [np.nan]` which is `False`, so the function returns `True` when it shouldn't.I've been getting dynesty errors from nan loglikelihoods during construction of the Dynesty sampler object. I was stepping through the bilby internals with pdb and was finding that the nan loglikelihoods were happening in `get_initial_points_from_prior` in the `base_sampler.py`. I was confused as to why the function `check_draw` wasn't preventing `theta`'s which produce nan loglikelihoods from being added to the returned list. I believe the reason is a bug in `check_draw`, which has the following implementation.
```
def check_draw(self, theta, warning=True):
bad_values = [np.inf, np.nan_to_num(np.inf), np.nan]
if abs(self.log_prior(theta)) in bad_values:
if warning:
logger.warning('Prior draw {} has inf prior'.format(theta))
return False
if abs(self.log_likelihood(theta)) in bad_values:
if warning:
logger.warning('Prior draw {} has inf likelihood'.format(theta))
return False
return True
```
However, nan's don't behave as other values with respect to equality. `nan != nan` is `True`, so if `self.log_likelihood(theta)` is nan, then the check becomes `abs(np.nan) in [np.nan]` which is `False`, so the function returns `True` when it shouldn't.https://git.ligo.org/lscsoft/bilby/-/issues/548Issue with injection parameters in run_sampler2020-12-10T13:41:50ZGregory Ashtongregory.ashton@ligo.orgIssue with injection parameters in run_samplerEmail from Stephen Feeney:
> It's nothing enormous, but it can result in the results.injection_parameter storing the wrong inclination angle, iota. This happens for me because I'm using a reference frequency that is different from bilby's default. In bilby/core/sampler/__init__.py, in run_sampler, when appending the injection_parameters to the result object, conversion_function is called without passing in the likelihood object (which in turn contains the user-specified minimum and reference frequencies). This means result.injection_parameters uses the default reference_frequency when converting theta_jn etc. into iota, and as a result my results objects' iotas don't match the ones actually used when injected signals. Niche :) But I thought you'd want to know :)
Pointing to the code, [this line](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/sampler/__init__.py#L209) needs to also pass in the [likelihood](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/gw/conversion.py#L758).
However, this creates a bit of a problem: if users pass in their own conversion functions which don't accept the likelihood it will fall over.
Fundamentally, I think we are missing a standard interface for the "conversion" function. I think the resolution here is to set a convention for the conversion function. What do folks think?Email from Stephen Feeney:
> It's nothing enormous, but it can result in the results.injection_parameter storing the wrong inclination angle, iota. This happens for me because I'm using a reference frequency that is different from bilby's default. In bilby/core/sampler/__init__.py, in run_sampler, when appending the injection_parameters to the result object, conversion_function is called without passing in the likelihood object (which in turn contains the user-specified minimum and reference frequencies). This means result.injection_parameters uses the default reference_frequency when converting theta_jn etc. into iota, and as a result my results objects' iotas don't match the ones actually used when injected signals. Niche :) But I thought you'd want to know :)
Pointing to the code, [this line](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/sampler/__init__.py#L209) needs to also pass in the [likelihood](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/gw/conversion.py#L758).
However, this creates a bit of a problem: if users pass in their own conversion functions which don't accept the likelihood it will fall over.
Fundamentally, I think we are missing a standard interface for the "conversion" function. I think the resolution here is to set a convention for the conversion function. What do folks think?https://git.ligo.org/lscsoft/bilby/-/issues/537Bugs in reweighing samples2020-10-19T19:05:23ZKa-Lok LoBugs in reweighing samplesHi,
While trying to reweigh samples using the routine `bilby.core.result.reweight`, I encountered two bugs (in `bilby-v1.0.2`)
1. In `bilby.core.result.get_weights_for_reweighting`, it assumed that the label for each row in `result.posterior` ranges from [0, len(result.posterior)-1] which is _not necessarily the case_ especially rejection sampling was used prior to calling this `get_weights_for_reweighting` function. This will cause index-out-of-bound error. The simplest fix would be to change in [L123 of bilby/core/result.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/core/result.py#L123) from
```
for ii, sample in result.posterior.iterrows():
```
to
```
for ii, (_, sample) in enumerate(result.posterior.iterrows()):
```
and also changing [L174 of bilby/core/result.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/core/result.py#L174)
from
```
return posterior[keep]
```
to
```
return posterior[keep].reset_index(drop=True)
```
2. It seems that the action of doing `convert_to_flat_in_component_mass_prior` does not commute with doing `bilby.core.result.get_weights_for_reweighting`. Doing reweighing before converting to flat in component mass prior yields no error but doing in the reverse order will yield an error saying that the key 'mass_1' is missing when the column actually exists. This is not desirable since `convert_to_flat_in_component_mass_prior` can be configured to run right after sampling, thus prohibiting reweighing of the samples in the future. This can be solved by properly assigning the component masses as one of the searched parameters and chirp mass and mass ratio being constrained in the result.search_parameter_keys and result.constraint_parameter_keys respectively. For example [starting from L67 of bilby/gw/prior.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/gw/prior.py#L67) can be changed from
```
for key in ['chirp_mass', 'mass_ratio']:
priors[key] = Constraint(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label)
for key in ['mass_1', 'mass_2']:
priors[key] = Uniform(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label,
unit="$M_{\odot}$")
```
to
```
for key in ['chirp_mass', 'mass_ratio']:
priors[key] = Constraint(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label)
result.constraint_parameter_keys.append(key)
result.search_parameter_keys.remove(key)
for key in ['mass_1', 'mass_2']:
priors[key] = Uniform(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label,
unit="$M_{\odot}$")
result.search_parameter_keys.append(key)
result.constraint_parameter_keys.remove(key)
```
3.(?) Probably the function should be called `reweigh` instead of `reweight` but whatever
If people prefer, I can submit a merge request fixing issue 1 and 2 or it would be faster if the developers simply affect these simple changes.Hi,
While trying to reweigh samples using the routine `bilby.core.result.reweight`, I encountered two bugs (in `bilby-v1.0.2`)
1. In `bilby.core.result.get_weights_for_reweighting`, it assumed that the label for each row in `result.posterior` ranges from [0, len(result.posterior)-1] which is _not necessarily the case_ especially rejection sampling was used prior to calling this `get_weights_for_reweighting` function. This will cause index-out-of-bound error. The simplest fix would be to change in [L123 of bilby/core/result.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/core/result.py#L123) from
```
for ii, sample in result.posterior.iterrows():
```
to
```
for ii, (_, sample) in enumerate(result.posterior.iterrows()):
```
and also changing [L174 of bilby/core/result.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/core/result.py#L174)
from
```
return posterior[keep]
```
to
```
return posterior[keep].reset_index(drop=True)
```
2. It seems that the action of doing `convert_to_flat_in_component_mass_prior` does not commute with doing `bilby.core.result.get_weights_for_reweighting`. Doing reweighing before converting to flat in component mass prior yields no error but doing in the reverse order will yield an error saying that the key 'mass_1' is missing when the column actually exists. This is not desirable since `convert_to_flat_in_component_mass_prior` can be configured to run right after sampling, thus prohibiting reweighing of the samples in the future. This can be solved by properly assigning the component masses as one of the searched parameters and chirp mass and mass ratio being constrained in the result.search_parameter_keys and result.constraint_parameter_keys respectively. For example [starting from L67 of bilby/gw/prior.py](https://git.ligo.org/lscsoft/bilby/-/blob/1.0.2/bilby/gw/prior.py#L67) can be changed from
```
for key in ['chirp_mass', 'mass_ratio']:
priors[key] = Constraint(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label)
for key in ['mass_1', 'mass_2']:
priors[key] = Uniform(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label,
unit="$M_{\odot}$")
```
to
```
for key in ['chirp_mass', 'mass_ratio']:
priors[key] = Constraint(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label)
result.constraint_parameter_keys.append(key)
result.search_parameter_keys.remove(key)
for key in ['mass_1', 'mass_2']:
priors[key] = Uniform(priors[key].minimum, priors[key].maximum, key, latex_label=priors[key].latex_label,
unit="$M_{\odot}$")
result.search_parameter_keys.append(key)
result.constraint_parameter_keys.remove(key)
```
3.(?) Probably the function should be called `reweigh` instead of `reweight` but whatever
If people prefer, I can submit a merge request fixing issue 1 and 2 or it would be faster if the developers simply affect these simple changes.https://git.ligo.org/lscsoft/bilby/-/issues/524Issue with caching in bilby.hyper.model2020-09-28T15:24:54ZShanika GalaudageIssue with caching in bilby.hyper.modelThe current method does not check if the input data is the same
e.g.
```bash
prob1 = hyper_model.prob(data1)
prob2 = hyper_model.prob(data2)
```
will return the same probabilities.
A data check is required [here](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/hyper/model.py#L28).The current method does not check if the input data is the same
e.g.
```bash
prob1 = hyper_model.prob(data1)
prob2 = hyper_model.prob(data2)
```
will return the same probabilities.
A data check is required [here](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/hyper/model.py#L28).https://git.ligo.org/lscsoft/bilby/-/issues/523Bug in hyper model2020-09-29T00:55:34ZNikhil SarinBug in hyper modelThe first term in the hyper model should be "dataset" rather than "data". I don't know if this is just on my end yet but the hyper example is broken without this change.The first term in the hyper model should be "dataset" rather than "data". I don't know if this is just on my end yet but the hyper example is broken without this change.https://git.ligo.org/lscsoft/bilby/-/issues/504Joint distributions difficult to subclass.2020-07-13T19:09:04ZColm Talbotcolm.talbot@ligo.orgJoint distributions difficult to subclass.I'm trying to subclass the `GaussianMixture` and `GaussianMixtureDist` classes and ran into an issue with the way the underlying distribution is checked. I think that [this line](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/prior/joint.py#L669) should be changed to
```python
if not isinstance(dist, BaseJointPriorDist):
```
Unless I'm missing something @matthew-pitkin @bruce.edelman, I think you're most familiar with this bit of the code?
EDIT:
I'm now having an issue reading the prior from a standard `.prior` file. There are specifically [enumerated names](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/prior/dict.py#L217). We should also look for subclasses.I'm trying to subclass the `GaussianMixture` and `GaussianMixtureDist` classes and ran into an issue with the way the underlying distribution is checked. I think that [this line](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/prior/joint.py#L669) should be changed to
```python
if not isinstance(dist, BaseJointPriorDist):
```
Unless I'm missing something @matthew-pitkin @bruce.edelman, I think you're most familiar with this bit of the code?
EDIT:
I'm now having an issue reading the prior from a standard `.prior` file. There are specifically [enumerated names](https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/prior/dict.py#L217). We should also look for subclasses.https://git.ligo.org/lscsoft/bilby/-/issues/484sys.exit in jupyter2020-05-08T03:13:08ZColm Talbotcolm.talbot@ligo.orgsys.exit in jupyterI'm running into issues stopping jobs in Jupiter. When I interrupt `dynesty` it triggers a call to `sys.exit` which kills the kernel.
Maybe we should avoid calling `sys.exit` and instead raise a custom error that can be caught and then the exit can be called by downstream users.I'm running into issues stopping jobs in Jupiter. When I interrupt `dynesty` it triggers a call to `sys.exit` which kills the kernel.
Maybe we should avoid calling `sys.exit` and instead raise a custom error that can be caught and then the exit can be called by downstream users.