Skip to content
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

Update nested sampler integration #348

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Update nested sampler integration #348

merged 4 commits into from
Oct 1, 2024

Conversation

johannesulf
Copy link
Contributor

This PR improves the nested sampler integration. The API now allows more direct communication between the user and the sampler by passing keyword arguments directly to the sampler function calls. Prospector now looks at the signatures of function calls in the different sampler packages and dynamically assigns the appropriate keyword arguments passed by the user. At the same time, the following keyword arguments remain "global", i.e., prospector translates them into the corresponding keyword arguments for the respective sampler.

  • nested_neff: The minimum effective sample size.
  • nested_nlive: The number of live points.
  • verbose: Whether to give progress updates.

I also ended up removing the keyword arguments nested_bound, nested_sample, nested_walks, and nested_dlogz. My reason was that it was somewhat confusing that they had slightly different names than in dynesty. Plus, they don't always have a corresponding parameter in the other samplers. The main practical advantage of the new implementation is that prospector doesn't needs to follow changing APIs in the samplers. For example, if a sampler implements a new keyword argument, users can directly utilize it without having to update prospector. Finally, whether parallelization with MPI works as expected for every sampler may still need to be checked.

@bd-j
Copy link
Owner

bd-j commented Sep 26, 2024

Thank you @johannesulf! I think the argument inspection will be very useful. I'll pull a version and make sure tests_sampler runs.

@bd-j
Copy link
Owner

bd-j commented Oct 1, 2024

So, I gave this a try with tests_samplers.py with minor changes, and it seems to run well. My only concern is this line gives a potentially huge number of not always helpful warnings, since in common practice the full kwargs dictionary often includes parameters related to other aspects of the code.

I understand passing non-relevant keywords is not great practice, but until it is improved in many disparate parts of the code I wonder if there could be something like an additional 'debug' or 'validate_sampling_keywords` variable that defaults to False to keep this line from running?

Happy to accept this PR and make such a change afterwards.

@johannesulf
Copy link
Contributor Author

Yes, totally agreed. I tried out the code and also noticed the warnings. When coding this up, I worked under the assumption that only sampler keyword arguments would be passed here. Happy to have this line removed or adjusted according to your suggestion.

@bd-j bd-j merged commit e3182c0 into bd-j:main Oct 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants