- Notifications
You must be signed in to change notification settings - Fork180
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Refactorsimulate_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 thesimulate_for_sbi function are not covered in tests.
Would you be up for writing a test that testssimulate_for_sbi with different cases, e.g., withnum_simulations=0,simulation_batch_size=None,num_workers>1, usingpytest.mark.parametrize?
You could just use alinear_gaussian simulator as in the other tests.
Ideally, you would also add one case where we runprocess_simulator and one where we don't, to cover theTypeError case whennum_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 theTypeError handling, we just need to cover the case where the user passes a torch simulator withnum_workers>1 without processing this simulator usingprocess_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 <abdulsamadsid1@gmail.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
Signed-off-by: samadpls <abdulsamadsid1@gmail.com>
e80f9ac to39a83b9CompareSigned-off-by: samadpls <abdulsamadsid1@gmail.com>
39a83b9 to47768a3CompareThere 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
xin 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)