-
Notifications
You must be signed in to change notification settings - Fork 26
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 html repr #883
Add html repr #883
Conversation
I believe the commit I added in the pull request did this and should be inherited. Not quite sure how to test this though. |
ah, ok. I'll take a look |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #883 +/- ##
==========================================
- Coverage 88.20% 88.17% -0.04%
==========================================
Files 44 44
Lines 9032 9067 +35
Branches 2587 2595 +8
==========================================
+ Hits 7967 7995 +28
- Misses 752 757 +5
- Partials 313 315 +2
☔ View full report in Codecov by Sentry. |
The triangles aren't showing up for me, but I still think this looks pretty good |
Hm, I do think the triangles are quite useful to indicate you can "open a level". Are you still able to go down levels without the triangles? |
Yes, and I changed the styles so the bold indicates expandable. I like the triangles too. I'm not sure why they aren't showing up for me. I'm not an HTML/CSS expert but I'd be happy to run anything you want to try. I'm using Chrome in a Jupyter notebook on a mac. |
My guess is that the font you are using is not supported so the browser falls back on a different font. I.e., I suspect the issue is with
I agree, that's pretty cool.
I think we probably also would want to add a |
I'm still not getting triangles even with the new font |
I few other minor points:
This is almost good, but we should dynamically set the text of the title based on the name of the object. I have changed it to the following rule: if self.__class__.__name__ == "NWBFile":
nwb_header_text = "NWBFile"
elif self.name == self.__class__.__name__:
nwb_header_text = self.name
else:
nwb_header_text = f"{self.name} ({self.__class__.__name__})"
However, there is currently an edge-case when you have large datasets stored in Python native This won't be an issue for any dataset that has been written and read back from disk, so I don't think it's a huge issue, but just something to be aware of |
src/hdmf/container.py
Outdated
if self.__class__.__name__ == "NWBFile": | ||
nwb_header_text = "NWBFile" |
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.
Can you clarify why this first part of this if statement to check for "NWBFile" as a name is necessary? Isn't this covered by the second part? Also, it seems strange to check for NWBFile in HDMF.
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 not covered by the second part because the root NWBFile
object technically has the name
"root".
Another option is to take this clause out let the name title of an NWBFile object be: "root (NWBFile)".
I suppose another option could be to create a method for this and override it in the NWBFile
class.
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 the name title of an NWBFile object be: "root (NWBFile)"
I think that would be fine. We can always customize in PyNWB if necessary. Alternatively you could also check for:
if self.name == 'root'`:
nwb_header_text = self.__class__.__name__
but to be honest, I would go with consistency here and just let be root (NWBFile)
A more nit-picky comment. I would suggest to remove the |
With respect to the triangle, it should just be rendered via the html tag details (reference here), so it shouldn't be anything fancy. For me it works in jupyter lab in both Chrome and Firefox. TestTest |
Based on this stack overflow, @bendichter can you try the following CSS: CSS_STYLE = """
<style>
.container-fields {
font-family: "Open Sans", Arial, sans-serif;
}
.container-fields .field-value {
color: #00788E;
}
.container-fields details > summary {
cursor: pointer;
display: list-item;
}
.container-fields details > summary:hover {
color: #0A6EAA;
}
</style>
""" |
unfortunately that did not do the trick. |
That looks good to me. Looks like all this needs is a unit test and it is good to go |
I think this is a good checkpoint. It provides clear value above what we have before and works reasonably well. I also think there are lots of opportunities for improvements, e.g.
The nice thing about this implementation is that downstream classes can implement their own custom html |
@oruebel what are you envisioning for a test for this feature? |
@oruebel I added the very basic: def test_repr_html_(self):
assert isinstance(Container('obj1')._repr_html_(), str) which ensures that |
I think having a test that checks string equivalence for a Container that covers more of the different cases in the function would be great. |
Looks good to me. Thanks for this great new feature. |
Thanks @edeno for the enhancement! |
Motivation
Inspired by the handy html reprs provided by dask, xarray, and pandas (see this blog post for some examples), I wanted to create an easy visual representation of the pynwb file that allows users to:
nwb_file.fields["processing"]["behavior"].fields["data_interfaces"][ "finger_pos" ].fields["starting_time"]
)This visualization only displays when using a jupyter notebook, but I believe enough users use these tools for it to be of use. While this does overlap with some of the functionality of NWB Widgets, it is lighter-weight, requires no additional installation, and does no plotting. The goal is for the user to figure out what is in the contents of the nwb file and access the data quickly.
The code below should show off the functionality for an arbitrary NWB file, although the copying of the tooltip is still a work in progress.
How to test the behavior?
Example display