[PolyChord] use ppf for pc_prior instead of prior bounds #104
Conversation
| @@ -180,7 +180,7 @@ def initialize(self): | |||
| locs = bounds[:, 0] | |||
| scales = bounds[:, 1] - bounds[:, 0] | |||
| # This function re-scales the parameters AND puts them in the right order | |||
| self.pc_prior = lambda x: (locs + np.array(x)[self.ordering] * scales).tolist() | |||
| self.pc_prior = lambda x: [self.model.prior.pdf[i].ppf(xi) for i, xi in enumerate(np.array(x)[self.ordering])] | |||
There was a problem hiding this comment.
The pdf.ppf thing might read a bit confusing. The thing is that prior.pdf is really a scipy.stats.rv_continuous class that has .pdf, .logpdf and .ppf as functions. Might be worth considering to rename prior.pdf to something else? (e.g. prior.rv_continuous or prior.distributions?)
* Use scipy's .ppf (inverse cdf) that comes with all major 1d distributions for PolyChord's prior function, which needs to map the unit hypercube to the physical parameters. Previously the prior bounds were used to effectively turn any 1d prior into a uniform prior. That's fine for the evidence, but causes problems e.g. for the relative entropy from prior to posterior. * Pass the loglikelihood to PolyChord instead of logposterior+logvolume.
| @@ -180,7 +180,7 @@ def initialize(self): | |||
| locs = bounds[:, 0] | |||
| scales = bounds[:, 1] - bounds[:, 0] | |||
There was a problem hiding this comment.
There might remain some possible cleaning up to do, e.g. I expect that self.logvolume won't be needed anymore, right? Consequently also scales and locs can be removed. Maybe we don't even need bounds here anymore? Meaning we could even get rid of all lines from 170 to 181.
Or is there a different reason that PolyChord priors need to be bounded that isn't obvious to me? @williamjameshandley, maybe you can confirm whether by using the .ppf we indeed don't need to specify prior bounds anymore?
There was a problem hiding this comment.
Certainly most other interfaces to PolyChord don't put in these bounds, so if it is necessary it might be cobaya specific. These changes all look good to me, so it's up to @JesusTorrado to approve/comment on this.
| f_paramnames.write("%s*\t%s\n" % ( | ||
| "logprior" + _separator + p, | ||
| r"\pi_\mathrm{" + p.replace("_", r"\ ") + r"}")) | ||
| r"\log\pi_\mathrm{" + p.replace("_", r"\ ") + r"}")) |
There was a problem hiding this comment.
Would be good to add this change to #233.
Hi @JesusTorrado,
I gave this a shot and it seems to be a fairly minimal change. But I'm obviously not super familiar with the internals of Cobaya, hence I am not sure whether I might be overlooking something.
This implements the changes suggested in #101 in order to get accurate Kullback-Leibler divergences also in case of non-uniform priors.
The
pc_priorfunction now uses the.ppffunction of thescipy.stats1-dimensional distributions.Fixes #101
Fixes #103