-
Notifications
You must be signed in to change notification settings - Fork 686
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
bug fixes and jupyter notebook support added #212
Conversation
Codecov Report
@@ Coverage Diff @@
## master #212 +/- ##
==========================================
- Coverage 95.48% 95.19% -0.30%
==========================================
Files 12 12
Lines 909 915 +6
Branches 180 184 +4
==========================================
+ Hits 868 871 +3
- Misses 15 18 +3
Partials 26 26
Continue to review full report at Codecov.
|
cleanlab/internal/util.py
Outdated
shell = get_ipython().__class__.__name__ | ||
if shell == "ZMQInteractiveShell": | ||
display(df) # Jupyter notebook or qtconsole | ||
elif shell == "TerminalInteractiveShell": | ||
print(df) # terminal, not notebook | ||
else: | ||
print(df) # something else, but not notebook |
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.
An alternative could be: just display(df)
in L460 and delete these extra checks. They anyway just goto L470, which would happen anyway if try
clause fails.
I ask because not sure if ZMQInteractiveShell
is overly limiting. Maybe display(df)
works in other envs as well?
Should keep as is if there's some specific situation where display(df)
will not cause try
to fail, but is not desirable to execute?
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 from my terminal console (shell = TerminalInteractiveShell), both display(df) and print(df) work fine. They just give same output.
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.
Eg. in colab, I don't think shell == "ZMQInteractiveShell"
but display(df) still would be preferred over print(df)
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.
Agree, but similarly do not know the answer to your last question. I can make this change, but it makes exception handling more challenging for developers in the future if we wish to expand this functionality.
EDIT: i see for some terminals that have jupyter, get_ipython().__class__.__name__
returns Shell
, so will change to try: display(df)
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.
to your second and third comment, we still need error checking because in my console, i don't have IPython installed so it throws an error if i try to display
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, just suggested possible minor improvement of the display(df) part
This PR adds three things to
dataset.health_summary
health_summary
in a terminal with no jupyter notebook, it should still print to console.pred_probs
is not provided.