-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fixing fraud cost decision function #254
Conversation
Codecov Report
@@ Coverage Diff @@
## master #254 +/- ##
==========================================
- Coverage 97.32% 97.07% -0.26%
==========================================
Files 95 95
Lines 2731 2731
==========================================
- Hits 2658 2651 -7
- Misses 73 80 +7
Continue to review full report at Codecov.
|
@@ -46,7 +46,7 @@ def decision_function(self, y_predicted, extra_cols, threshold): | |||
if not isinstance(y_predicted, pd.Series): | |||
y_predicted = pd.Series(y_predicted) | |||
|
|||
transformed_probs = (y_predicted * extra_cols[self.amount_col]) | |||
transformed_probs = (y_predicted.values * extra_cols[self.amount_col]) |
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's add/change a test case that would have caught this
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.
Updated all tests for Auto(*) so that the raise_error flag is set to True. Unsure if there's a better alternative (that doesn't require us to remember to set this?)
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.
one thought would be that we could create a global raise_errors
setting. perhaps set with an env variable. i like what you did for now though.
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
@@ -46,7 +46,7 @@ def decision_function(self, y_predicted, extra_cols, threshold): | |||
if not isinstance(y_predicted, pd.Series): | |||
y_predicted = pd.Series(y_predicted) | |||
|
|||
transformed_probs = (y_predicted * extra_cols[self.amount_col]) | |||
transformed_probs = (y_predicted.values * extra_cols[self.amount_col]) |
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.
one thought would be that we could create a global raise_errors
setting. perhaps set with an env variable. i like what you did for now though.
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 fix looks good to me :)
How did we end up catching this bug? And how did we not catch it sooner? I ask purely because perhaps the answer would point out a place where we're missing test coverage.
@dsherry I caught the bug purely by coincidence when I was trying to figure out why catboost wasn't working on circleci and was clicking around the docs. We probably didn't catch it earlier because |
Oh, oops, I missed all of those changes! I must've been looking at a single commit. Cool. I do have more comments but I'll create a discussion ticket for them. |
Closes #252