joshspeagle / dynesty

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

Fix pool docs to clarify the sampler should not get its own args/kwargs #464

Closed eteq closed 8 months ago

eteq commented 8 months ago

Reference issue

N/A

What does you PR implement/fix ?

It took me a while to figure this one out. When trying to use the dynesty built-in pool helper I kept getting the following traceback:

Traceback (most recent call last):
  File "<REDACTED>/dynesty/dynesty.py", line 910, in __call__
    return self.func(np.asarray(x).copy(), *self.args, **self.kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: loglike_cache() takes 1 positional argument but 3 were given

it turns out this was happening because the top-level `__call__ method kept trying to pass in the args, but the pool expects no args and kwargs to the log likelihood since it caches them. I had naively just copied-and-pasted my sampler initialization code and done what the docstring said (updated the ll and ptrans to point to the pool), but didn't realize that the pool would also be taking care of the args and kwargs.

So this PR just clarifies that a bit more in the docs that this is a requirement not a "you can if you want to" as it was previously phrased.

A further step might be to actually recognize this failure mode and produce an exception in the initialization process. I figured this doc update is useful regardless, but if that sounds good to others I am happy to make an issue or maybe even a PR to implement it.

coveralls commented 8 months ago

Pull Request Test Coverage Report for Build 7170546226


Totals Coverage Status
Change from base Build 7105543502: 0.0%
Covered Lines: 4161
Relevant Lines: 4538

💛 - Coveralls
segasai commented 8 months ago

Thanks for the report @eteq!

I've made a tentative different fix though ( #466). so your code should just work. I.e. if you specify the logl_args in the sampler those will be used, if you specify the ones in the pool those will be used, and if you specify both in the pool and sampler those will be combined, which is I think sensible behaviour and the most user friendly.

eteq commented 8 months ago

Thanks @segasai ! I agree #466 is an even better solution (although see a minor clarification in https://github.com/joshspeagle/dynesty/pull/466#pullrequestreview-1776079414)

segasai commented 8 months ago

I merged the #466. Closing this. Thanks for spotting the issue.