bilby issueshttps://git.ligo.org/lscsoft/bilby/-/issues2021-03-18T05:26:20Zhttps://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/544dynesty "extremely inefficient" warnings only appear at end of log file2020-11-27T00:46:11ZDavid Keiteldavid.keitel@ligo.orgdynesty "extremely inefficient" warnings only appear at end of log fileI did some test runs with `rwalk_dynesty` setting and it turned out the first were badly tuned; `dynesty` actually through warnings about the walks being `extremely inefficient` but those only got printed in bulk at the very end of the log file, after the run finished, not when they occurred. So I suspect there's a missing flush of some sort somewhere.
My use case is parallel_bilby, but I'm opening the issue here since I assume it's calling dynesty by way of the main bilby package, or at least the same workaround would go into both...?
Reported before to @gregory.ashton who thinks this could be fixed on bilby's side.I did some test runs with `rwalk_dynesty` setting and it turned out the first were badly tuned; `dynesty` actually through warnings about the walks being `extremely inefficient` but those only got printed in bulk at the very end of the log file, after the run finished, not when they occurred. So I suspect there's a missing flush of some sort somewhere.
My use case is parallel_bilby, but I'm opening the issue here since I assume it's calling dynesty by way of the main bilby package, or at least the same workaround would go into both...?
Reported before to @gregory.ashton who thinks this could be fixed on bilby's side.https://git.ligo.org/lscsoft/bilby/-/issues/543fix linebreaks in dynesty logging2021-03-25T16:58:47ZDavid Keiteldavid.keitel@ligo.orgfix linebreaks in dynesty loggingUsing dynesty through `parallel_bilby`, the logfile entries of the `iter: 19 | bound: 0 | nc: 1 | ncall:` type don't seem to have proper unix line-breaks, showing up as `^M` in several tools including `less` and `emacs`, which is at least a mild annoyance when parsing them by eye or scripts.
I'm opening the issue here since @gregory.ashton mentioned this should be on the side of the main bilby package, not parallel.Using dynesty through `parallel_bilby`, the logfile entries of the `iter: 19 | bound: 0 | nc: 1 | ncall:` type don't seem to have proper unix line-breaks, showing up as `^M` in several tools including `less` and `emacs`, which is at least a mild annoyance when parsing them by eye or scripts.
I'm opening the issue here since @gregory.ashton mentioned this should be on the side of the main bilby package, not parallel.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/517Improve signal handling2020-08-19T08:04:41ZColm Talbotcolm.talbot@ligo.orgImprove signal handlingCurrently, we set the signal handler in the `__init__` methods for the samplers, e.g., https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/sampler/dynesty.py#L176-178.
These typically lead to calling a checkpoint function which only really makes sense during sampling.
This leads to some weird behaviour if the signals come after the sampler finishes or before the setup is complete. I've noticed this when jobs are interrupted during parameter reconstruction.
I suggest that instead we should set up the handler immediately before launching the sampler and change them back after the sampling is done.
We may also want to look at having a specific handler for the parameter reconstruction in the GW case.Currently, we set the signal handler in the `__init__` methods for the samplers, e.g., https://git.ligo.org/lscsoft/bilby/-/blob/master/bilby/core/sampler/dynesty.py#L176-178.
These typically lead to calling a checkpoint function which only really makes sense during sampling.
This leads to some weird behaviour if the signals come after the sampler finishes or before the setup is complete. I've noticed this when jobs are interrupted during parameter reconstruction.
I suggest that instead we should set up the handler immediately before launching the sampler and change them back after the sampling is done.
We may also want to look at having a specific handler for the parameter reconstruction in the GW case.https://git.ligo.org/lscsoft/bilby/-/issues/505Use NS weights for posterior plots and quantiles2020-09-28T15:14:12ZMatthew David PitkinUse NS weights for posterior plots and quantilesIf working with the outputs of nested sampling code and we have the weights stored in the `Result` object we should allow the `plot_corner` function to use all the nested samples, but with weights applied (as is done in [dynesty](https://github.com/joshspeagle/dynesty/blob/master/dynesty/plotting.py#L968) and ultranest). This will reduce the sampling noise especially in the wings of the posteriors. We should also look at the dynesty [quantile](https://github.com/joshspeagle/dynesty/blob/master/dynesty/utils.py#L184) function, which will use the weights to work out quantiles, which again should reduce the random noise on the values.If working with the outputs of nested sampling code and we have the weights stored in the `Result` object we should allow the `plot_corner` function to use all the nested samples, but with weights applied (as is done in [dynesty](https://github.com/joshspeagle/dynesty/blob/master/dynesty/plotting.py#L968) and ultranest). This will reduce the sampling noise especially in the wings of the posteriors. We should also look at the dynesty [quantile](https://github.com/joshspeagle/dynesty/blob/master/dynesty/utils.py#L184) function, which will use the weights to work out quantiles, which again should reduce the random noise on the values.Futurehttps://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.