C-bowman / inference-tools

Python tools for Bayesian data analysis
MIT License
28 stars 3 forks source link

make clear that 'theta' argument in any user's posterior class must allow np.ndarray #4

Closed jsallcock closed 4 years ago

jsallcock commented 4 years ago

This is a very minor point, but it gave me a few minutes of head-scratching.

I think the docs should make clear that when a chain takes a step, it casts the list of model parameters theta to np.ndarray. This means that a user's posterior class cannot call any list-only methods (such as list.pop() ) on theta. This behaviour is obviously fine, but i think it should be made clearer. Thanks.

jsallcock commented 4 years ago

I tried to reference a specific line of code there but it doesn't seem to have worked.

Here are lines 993-1004 of mcmc.py, which call self.posterior(prop) with prop as an array.

def take_step(self):
        """
        Take a Metropolis-Hastings step along each principal component
        """
        p_old = self.probs[-1]
        theta0 = array([p.samples[-1] for p in self.params])
        # loop over each eigenvector and take a step along each
        for v, p in zip(self.directions,self.params):
            while True:
                prop = theta0 + v*p.sigma*normal()
                prop = self.process_proposal(prop)
                p_new = self.posterior(prop) * self.inv_temp
C-bowman commented 4 years ago

Good point - I've now updated the docstrings for all the samplers in inference.mcmc to say specifically that model parameters are passed to posterior functions as a numpy.ndarray.

This change won't actually appear in the online docs until a new release is made however, so I'll leave this open for now and close it once the release happens. I was planning a new release within the next couple of weeks anyway so shouldn't be too long.

C-bowman commented 4 years ago

Version 0.5.2 has now been released, so the changes should appear in the stable version of the online docs shortly.