sbi icon indicating copy to clipboard operation
sbi copied to clipboard

ImportanceSamplingPosterior.sample is missing show_progress_bars argument.

Open janfb opened this issue 1 year ago • 2 comments

Description:

Unlike other posterior classes in the SBI library, the ImportanceSamplingPosterior class sample method does not currently offer the option to enable or disable progress bars. This feature would be particularly useful for users who need to control output verbosity, especially when running simulations in environments where progress bars could clutter logs or are otherwise unnecessary.

https://github.com/sbi-dev/sbi/blob/3b1f34e3a5755f097652d8e54767881ad783d630/sbi/inference/posteriors/importance_posterior.py#L154-L162

The sample method internally decides to call either _sir_sample, which already includes a progress bar, or _importance_sample, which currently does not implement a progress bar. To maintain consistency across the library and to enhance user control over the interface, I suggest we introduce a parameter (e.g., show_progress_bar) in the sample method signature. This parameter should then control the display of progress bars in both _sir_sample and _importance_sample methods accordingly.

Proposed Changes

  • [ ] Implement progress bar in _importance_sample
  • [ ] Add a show_progress_bar boolean parameter to the sample method of ImportanceSamplingPosterior.
  • [ ] Ensure that this parameter controls the display of progress bars in both the _sir_sample and _importance_sample methods.

This enhancement would align ImportanceSamplingPosterior with the other posterior classes in terms of API consistency and usability.

janfb avatar Feb 09 '24 12:02 janfb

Can I be assigned this issue? 🙏😄

kevnster avatar Feb 09 '24 18:02 kevnster

Absolutely! Please feel free to create a PR. If you haven't already, check out the contribution guidelines 😊

Thanks a lot for tackling this! 🙏

janfb avatar Feb 09 '24 19:02 janfb