-
Notifications
You must be signed in to change notification settings - Fork 46
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 BinaryClassifierPrecisionEfficacy
metric
#714
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
==========================================
+ Coverage 94.88% 95.11% +0.22%
==========================================
Files 106 110 +4
Lines 4126 4275 +149
==========================================
+ Hits 3915 4066 +151
+ Misses 211 209 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
return scores | ||
|
||
@classmethod | ||
def compute_breakdown( |
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.
why is this a class method?
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.
It's a class method by definition:
SDMetrics/sdmetrics/single_table/base.py
Line 168 in 280a93e
@classmethod |
|
||
self._fit_preprocess(data, metadata) | ||
|
||
def _transform_preprocess(self, tables): |
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.
minor: we could just call transform
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.
Since it follows _fit_preprocess
and I wanted it to differ from fit
to prevent confusion with the ML model fix, I think we can keep it this way.
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 ML model was separated out though. I think _fit_preprocess confuses me as it makes me wonder if we are fitting on preprocessed data or fitting as part of the preprocess step
Thanks for the review @amontanez24. I addressed the comments in 791c7d5 and aafdd1e with some refactoring as you recommended |
@@ -32,7 +32,7 @@ def wrapper(*args, **kwargs): | |||
ipython_interpreter = str(get_ipython()) | |||
if 'ZMQInteractiveShell' in ipython_interpreter and 'iframe' in renderers: | |||
# This means we are using jupyter notebook | |||
pio.renderers.default = 'vscode' | |||
pio.renderers.default = 'iframe' |
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 found that this change was accidentally added in a previous PR, so I reverted it.
CU-86b3m9u4d
Resolve #711