-
Notifications
You must be signed in to change notification settings - Fork 602
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
Visualizer #1567
Visualizer #1567
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1567 +/- ##
==========================================
- Coverage 92.50% 91.89% -0.61%
==========================================
Files 201 204 +3
Lines 18617 18538 -79
==========================================
- Hits 17221 17035 -186
- Misses 1396 1503 +107
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Should we add flask as a dependency (not sure though why tests pass, maybe it is shipped with another dependency)? |
hub/visualizer/visualizer.py
Outdated
visualizer = _Visualizer() | ||
|
||
|
||
def visualize(ds: Dataset): |
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.
curious why we went with visualize(ds) as a function instead of a method of the Dataset object.
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 reason is to avoid handling additional dependencies at the dataset core level.
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.
that can still be done, the dependencies can be imported when the user tries to use the functionality, if they aren't installed, we can throw an exception, urging the user to pip install hub[visualizer]
@@ -31,6 +31,7 @@ | |||
"audio": ["miniaudio"], | |||
"gcp": ["google-cloud-storage", "google-auth", "google-auth-oauthlib"], | |||
"video": ["av"], | |||
"visualizer": ["IPython", "flask"], |
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.
you need to add these to common.txt too
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 dont think we need the IPython
dependency.
hub/visualizer/visualizer.py
Outdated
url = f"http://localhost:{_PORT}/{id}/" | ||
iframe = IFrame( | ||
f"https://app.dev.activeloop.ai/visualizer/hub?url={url}", | ||
width="100%", |
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.
we can perhaps allow users to pass height and width as optional arguments
@khustup please make sure that all the tests pass and coverage is up before merging. Thx! |
🚀 🚀 Pull Request
Checklist:
coverage-rate
upChanges
Added visualization of the datasets in the Jupyter notebook.
It opens the grid of the visualizer with the following controls: