-
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
Add unit tests for standard metric objectives #741
Conversation
Codecov Report
@@ Coverage Diff @@
## master #741 +/- ##
==========================================
+ Coverage 99.36% 99.38% +0.01%
==========================================
Files 151 151
Lines 5378 5529 +151
==========================================
+ Hits 5344 5495 +151
Misses 34 34
Continue to review full report at Codecov.
|
…to 619_obj_unit_tests
evalml/objectives/objective_base.py
Outdated
raise ValueError("Length of inputs is 0") | ||
if len(y_predicted) != len(y_true): | ||
raise DimensionMismatchError("Inputs have mismatched dimensions: y_predicted has shape {}, y_true has shape {}".format(len(y_predicted), len(y_true))) | ||
if np.any(np.isnan(y_true)) or np.any(np.isinf(y_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.
FYI I think you can say np.isnan(y_true).any()
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.
@dsherry Oh, I used np.any
because of our previous discussion about numpy methods being faster?
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.
Both call np.any
! np.isnan
will return a np.array
. I believenp.any(value)
and value.any()
call the same method. Also lol this was a total nit-pick on my part, feel free to ignore 😂
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.
@angela97lin this is great! It's nice to have this sort of thing in the codebase :) this will help people quickly diagnose problems with their data and problem setup. Nice going!
I left comments but I have two main suggestions. First, I left a discussion about what we should do for nans/infs in the input. Second, can you please also add this input validation to our custom objectives (fraud cost and lead scoring) too?
I could imagine us wanting something similar in the plot metrics, but let's handle that another time, in a separate PR.
Closes #619
Adds the following tests: