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

Refactor simulate_for_sbi location #1253

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

samadpls
Copy link
Contributor

What does this implement/fix? Explain your changes

Move simulate_for_sbi to utils

Does this close any currently open issues?

Fixes #1240

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • I have read and understood the contribution
    guidelines
  • I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have reported how long the new tests run and potentially marked them
    with pytest.mark.slow.
  • New and existing unit tests pass locally with my changes
  • I performed linting and formatting as described in the contribution
    guidelines
  • I rebased on main (or there are no conflicts with main)
  • For reviewer: The continuous deployment (CD) workflow are passing.

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.33%. Comparing base (8afd985) to head (47768a3).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1253      +/-   ##
==========================================
- Coverage   86.05%   78.33%   -7.73%     
==========================================
  Files         118      119       +1     
  Lines        8672     8685      +13     
==========================================
- Hits         7463     6803     -660     
- Misses       1209     1882     +673     
Flag Coverage Δ
unittests 78.33% <100.00%> (-7.73%) ⬇️

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

Files with missing lines Coverage Δ
sbi/inference/__init__.py 100.00% <100.00%> (ø)
sbi/inference/trainers/base.py 92.54% <100.00%> (+5.63%) ⬆️
sbi/utils/simulation_utils.py 100.00% <100.00%> (ø)

... and 28 files with indirect coverage changes

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

That looks good, thanks a lot!

Now that you have moved the code, codecov notices that parts of the simulate_for_sbi function are not covered in tests.

Would you be up for writing a test that tests simulate_for_sbi with different cases, e.g., with num_simulations=0, simulation_batch_size=None, num_workers>1, using pytest.mark.parametrize?

You could just use a linear_gaussian simulator as in the other tests.
Ideally, you would also add one case where we run process_simulator and one where we don't, to cover the TypeError case when num_workers>1.

Let me know whether anything is unclear, or if you prefer not adding this (then I can easily add it in a separate PR).

Thanks again, cheers,
Jan

@samadpls
Copy link
Contributor Author

Thanks a lot for the feedback!
I’ve added the tests for simulate_for_sbi covering various cases. Let me know if there’s anything else!

Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

Great! thanks for adding the test as well.

I added a couple of comments. Regarding the TypeError handling, we just need to cover the case where the user passes a torch simulator with num_workers>1 without processing this simulator using process_simulator before. In that case, a TypeError will occur, be handled internally and then raised again. See comment, and let me know if anything is unclear.
Thanks!

tests/inference_with_NaN_simulator_test.py Outdated Show resolved Hide resolved
tests/inference_with_NaN_simulator_test.py Outdated Show resolved Hide resolved
tests/inference_with_NaN_simulator_test.py Outdated Show resolved Hide resolved
tests/inference_with_NaN_simulator_test.py Outdated Show resolved Hide resolved
tests/inference_with_NaN_simulator_test.py Outdated Show resolved Hide resolved
@samadpls samadpls force-pushed the refactor/simulate_for_sbi-location branch from e80f9ac to 39a83b9 Compare September 2, 2024 19:20
@samadpls samadpls force-pushed the refactor/simulate_for_sbi-location branch from 39a83b9 to 47768a3 Compare September 2, 2024 19:49
Copy link
Contributor

@janfb janfb left a comment

Choose a reason for hiding this comment

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

Awesome, this looks good!
Thanks a lot 🙏

@janfb janfb merged commit 033a05e into sbi-dev:main Sep 3, 2024
6 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.

simulate_for_sbi lives in trainers
2 participants