-
Notifications
You must be signed in to change notification settings - Fork 160
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
Refactor simulate_for_sbi
location
#1253
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this 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
Thanks a lot for the feedback! |
There was a problem hiding this 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!
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
Signed-off-by: samadpls <[email protected]>
e80f9ac
to
39a83b9
Compare
Signed-off-by: samadpls <[email protected]>
39a83b9
to
47768a3
Compare
There was a problem hiding this 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 🙏
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 creatingthe 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.
guidelines
with
pytest.mark.slow
.guidelines
main
(or there are no conflicts withmain
)