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

fix: protocol and refactor for custom potential #1409

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

janfb
Copy link
Contributor

@janfb janfb commented Feb 24, 2025

fixes #1223 by introducing a protocol for custom potentials. This will expose the arguments that we expect for the custom potential and eliminates the need to explicitly check for arguments.

@janfb janfb requested a review from schroedk February 24, 2025 16:09
@janfb janfb self-assigned this Feb 24, 2025
@janfb janfb added the refactoring refactoring without API changes label Feb 24, 2025
@janfb janfb added this to the pre-hackathon release milestone Feb 24, 2025
Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 29.63%. Comparing base (18f92b1) to head (8408298).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
sbi/inference/potentials/base_potential.py 83.33% 1 Missing ⚠️

❌ Your project status has failed because the head coverage (29.63%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1409       +/-   ##
===========================================
- Coverage   89.31%   29.63%   -59.68%     
===========================================
  Files         119      119               
  Lines        8779     8914      +135     
===========================================
- Hits         7841     2642     -5199     
- Misses        938     6272     +5334     
Flag Coverage Δ
unittests 29.63% <87.50%> (-59.68%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sbi/inference/posteriors/base_posterior.py 61.72% <100.00%> (-25.93%) ⬇️
sbi/inference/potentials/base_potential.py 84.78% <83.33%> (-6.89%) ⬇️

... and 96 files with indirect coverage changes

Copy link
Contributor

@manuelgloeckler manuelgloeckler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for adding. Much cleaner then the old CallableWrapper.

Some minor docstring changes can be made, bot happy to merge it as is.

@janfb janfb merged commit 69b7f38 into main Mar 5, 2025
4 checks passed
@janfb janfb deleted the fix-1223-custom-potential branch March 5, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring refactoring without API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interchangeability of Callable and BasePotential
2 participants