-
Notifications
You must be signed in to change notification settings - Fork 14k
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: move post_processing to unittest #18779
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18779 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1620 1620
Lines 63087 63092 +5
Branches 6372 6372
=======================================
+ Hits 41840 41845 +5
Misses 19590 19590
Partials 1657 1657
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Love the change to proper unit tests! One question about the change in the contribution operator - should we potentially split this PR in two - the first for the refactor, and the second for the functional change once the refactor has been merged?
# replace infinity and nan with 0 in dataframe | ||
numeric_df.replace(to_replace=[np.Inf, -np.Inf, np.nan], value=0, inplace=True) |
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.
Are we sure we want to display infinities as 0? I think having 1/0
to result in null
to be more intuitive. I assume keeping infinities as null would cause a break in the chart as opposed to it dropping to zero, which IMO feels more intuitive.
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.
let us discuss this in new PR.
it's my bad. I'm gonna split it to 2 PRs. |
b4e7d78
to
ead18f1
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.
LGTM! 🚀
SUMMARY
This PR decouple post-processing unit test and move it from the integration_test to the unit_tests. In order to make life easy, replace class test cases with functional test cases.
ADDITIONAL INFORMATION