search

joshspeagle / dynesty

Dynamic Nested Sampling package for computing Bayesian posteriors and evidences
https://dynesty.readthedocs.io/
MIT License
347 stars 76 forks source link

KL divergence code is broken ? #308

Closed segasai closed 3 years ago

segasai commented 3 years ago

I've tried to add a test of a KL divergence here https://github.com/segasai/dynesty/blob/95c4b09fe36a0c6bed3633b383b30c5581d3137d/tests/test_misc.py#L124 and it currently fails. 

        kl = dyutil.kl_divergence(res2, res1)
>       assert (kl[-1] > 0)
E       assert -0.056839523328596174 > 0

I disabled the sign check temporarily but it needs to be fixed (looked into).

joshspeagle commented 3 years ago

Could this be the reason why? https://github.com/joshspeagle/dynesty/blob/243025cbfc5f1941e9dc1b00348c54f8b877bd6c/py/dynesty/utils.py#L1202-L1207

segasai commented 3 years ago

I don't quite know. I haven't investigated and the logic behind the code is not obvious to me.

joshspeagle commented 3 years ago

In theory, it's just running through the calculation:

kld = np.exp(logp1) * (logp1 - logp2) return np.cumsum(kld)

Since res2 is just a bootstrap resampled version of res1, it occupies a subset of the points from res1 but with different associated logXs (and therefore logwts). So the idea is to just go through and match up the points in res1 with the closest corresponding point in res2, compute the KLD at the corresponding point, and then just sum them up across all the points. All the relevant lines look like they're implementing the right calculation https://github.com/joshspeagle/dynesty/blob/243025cbfc5f1941e9dc1b00348c54f8b877bd6c/py/dynesty/utils.py#L1173 https://github.com/joshspeagle/dynesty/blob/243025cbfc5f1941e9dc1b00348c54f8b877bd6c/py/dynesty/utils.py#L1184 https://github.com/joshspeagle/dynesty/blob/243025cbfc5f1941e9dc1b00348c54f8b877bd6c/py/dynesty/utils.py#L1205-L1207 https://github.com/joshspeagle/dynesty/blob/243025cbfc5f1941e9dc1b00348c54f8b877bd6c/py/dynesty/utils.py#L1210

but there's probably something subtle happening that I'm just not seeing right now...

segasai commented 3 years ago

I guess what doesn't quite make sense to me why the code need to look at sample_id's at all here. IMO the match need to be just by position. And all the points need to be binned by repeats with repeats logp summed and after that the two equal length logp arrays compared. (but also since kl_divergence is not actually used anywhere in the code (or even examples), TBH there is some rationale to just yank that code.

joshspeagle commented 3 years ago

Yea looking back on it I actually think I had just done it to check results against something for some reason I forget. The scheme is...odd, and it doesn't really do much otherwise since I just use kld_error where it matters. I agree that we can just yank this.

joshspeagle commented 3 years ago

Should be resolved after merging #309.