-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for named parameters #386
Conversation
Thanks @tmcclintock: This looks like a good start! I'll take a closer look ASAP, but I think I'm overall very happy with this approach. I think the one thing I would like to have would be to allow each parameter to have an arbitrary |
Also: |
@dfm thanks for the quick response. RE each parameter have an arbitrary RE |
I'm not suggesting changing the syntax, I'm just saying that if you're adding names to elements of the vector space, there's no reason why there needs to be one name per dimension. My parameters are often multivariate, and it would be annoying to manually add If this isn't clear, I'll try to put together a little demo this week. |
I see, so for instance in a GMM you would essentially like to do: params["weights"].shape
# (2,) # component weights
params["means"].shape
# (2,)
params["cov"].shape
# (3,) # independent elements of the cov matrix Correct? For an MVP this is simple -- if we restrict keywords to be either single values or consecutive numbers in the parameter array then this is easy. If instead we wanted to allow a keyword to map to an arbitrary set of indices in the params array, this is tricky (like if Are you OK with allowing only consecutive parameters? |
Yes this sounds about right and I would be totally happy with requiring contiguous values! |
…her integers or lists of integers
…her integers or lists of integers. Also ran isort and black
@dfm I have updated the PR. Now the following pattern is supported: def ln_prob(params):
mean1, mean2 = params["means"]
var1, var2 = params["vars"]
constant = params["constant"]
...
sampler = EnsembleSampler(..., parameter_names = {"means": [0, 1], "vars": [2, 3], "constant": 4}) That is,
So, I was able to support non-contiguous parameter indexes. Let me know if this all looks good. If you'd like docs work done I can either do that here or in another PR. |
@dfm can you push the button on the workflow? I'd like to tackle any problems that might occur ahead of your review. |
@tmcclintock: Weird - I thought I already had. Maybe I have to do it each time? |
Don't worry about the coverage failure. |
@tmcclintock: Sorry about the delay on this! I think this looks awesome so I've merged and we can address issues as they come up in other threads. If you'd be up for adding a tutorial about this, that would be awesome. Want to start another PR whenever you have a chance? Thanks again for this! |
@dfm no problem, and sure, I'd be happy to. |
Sorry to revive this, but did the tutorial mentioned ever get put together? If so, could you point me to it? I can probably figure it out on my own, but I figured I would check. I'm super excited about this feature. Thanks! |
@cluhmann I have not written the tutorial. My bad :/. You can see an example of how to set up a function with named parameters in the unit test found in |
No worries. I figured it out (just being lazy), but the tests help. Thanks again for the feature! |
This PR closes #385 by giving support for named parameters in the
EnsembleSampler
.Specifically this PR:
str
but really can be anything) for the parameters to the constructor forEnsembleSampler
. Checks are made to ensure that the number of unique names arendim
EnsembleSampler.compute_log_prob
method, these names are used to reshape the parameters passed in to the method (having previously been generated by theMove
) so that instead of anndarray
of shape(nwalkers, ndim)
it becomes a list of lengthnwalkers
of dictionaries of lengthndim
, with each key-value pair being the names and parametersndarray
->List[Dict[str, np.number]]
conversion, so that it could be tested separately from thecompute_log_prob
method (higher testable surface area)src/emcee/tests/unit/test_ensemble.py
file. I tested the helper method,compute_log_prob
, andrun_mcmc
ensemble.py
to be more PEP8 compliant/more pythonicAs discussed in #385, this PR makes no assumptions about the kinds of likelihood functions or underlying models being utilized. It does not take gradients of the likelihood, nor does it break the API (frontend or backend) in any way. I believe the unit test in in the
TestNamedParametes
tests make it clear why this is useful. Positional bugs will be a thing of the past!Support for named parameters could go a bit further. For example:
vectorize=True
when naming the parameters. I guess the right thing to allow for would be ifvectorize=True
then we produce one dictionary where the keys are the parameter names and the values are 1D arrays of each parameter.PTSampler
. IMO doing this would be "double work" and from a software perspective it would make more sense (IMO) to make an abstractSampler
class that handled the names, and that both theEnsembleSampler
andPTSampler
would subclass. But, this is obviously a non-trivial refactor (that I would be happy to contribute to).Thanks @dfm for the discussion on #385. I look forward to your review.