-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Deprecate remaining VI methods #5806
Conversation
Hi @ferrine. Sorry about my previous PR; I believe I made a mistake when resolving merge conflicts and accidentally deleted that branch. I deleted several flow-related tests and refactored others, but there are still 10 tests that I am unable to refactor and have commented so that other tests can be checked. Could you please assist me in refactoring the tests I commented? |
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.
I've left a lot of comments, the PR is still in a draft state. I hope that helps
Codecov Report
@@ Coverage Diff @@
## main #5806 +/- ##
==========================================
+ Coverage 89.39% 89.89% +0.50%
==========================================
Files 74 73 -1
Lines 13767 13198 -569
==========================================
- Hits 12307 11865 -442
+ Misses 1460 1333 -127
|
Hello, @ferrine Thank you for your feedback; I worked on it and am finally finished with it.
I commented them out in the code above please have a look and also please let me know if I missed anything in the docstring. Thank you : ) |
I'll be on these issues in a day or two with a close look |
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.
The last test refactoring is required to go ahead. I've proposed a snippet to resolve the failing test
Sure @ferrine, Thank you. |
@purna135 want to join our dev sprint? https://discord.gg/Hpne6mtJ |
Joining in few min, Thank you |
I've created an issue to fix deterministic sampling for Empirical approx (#5837 ) |
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.
lgtm
Congrats on your first PR @purna135 -- thanks for your help! |
Addressing #5581