Skip to content
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.

Sign up for GitHub

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 custom html field generation method for TimeSeries #1831

Merged
merged 9 commits into from
Feb 9, 2024
Merged

add custom html field generation method for TimeSeries #1831

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4468878
add custom html field generation method for timeseries
stephprince Jan 24, 2024
d555919
update CHANGELOG.md
stephprince Jan 24, 2024
0ae1da2
Merge branch 'dev' into jupyter-recursion-error
rly Jan 24, 2024
fb4def6
modify html representation of linked timestamps and data
stephprince Jan 25, 2024
ee3af9e
add test for html representation of linked data
stephprince Jan 25, 2024
bce6858
update html representation of linked objects
stephprince Jan 29, 2024
d24bde8
update html representation test
stephprince Feb 8, 2024
5f38295
Merge branch 'dev' into jupyter-recursion-error
stephprince Feb 8, 2024
88c0d44
Use HDMF 3.12.2
rly Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
### Bug fixes
- Fix bug where namespaces were loaded in "w-" mode. @h-mayorquin [#1795](https://github.com/NeurodataWithoutBorders/pynwb/pull/1795)
- Fix bug where pynwb version was reported as "unknown" to readthedocs @stephprince [#1810](https://github.com/NeurodataWithoutBorders/pynwb/pull/1810)
- Fix recursion error in html representation generation in jupyter notebooks. @stephprince [#1831](https://github.com/NeurodataWithoutBorders/pynwb/pull/1831)

### Documentation and tutorial enhancements
- Add RemFile to streaming tutorial. @bendichter [#1761](https://github.com/NeurodataWithoutBorders/pynwb/pull/1761)
Expand Down
7 changes: 7 additions & 0 deletions src/pynwb/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,13 @@
def __add_link(self, links_key, link):
self.fields.setdefault(links_key, list()).append(link)

def _generate_field_html(self, key, value, level, access_code):
# reassign value if linked timestamp or linked data to avoid recursion error
if key in ['timestamp_link', 'data_link']:
value = {v.name: v.neurodata_type for v in value}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than link to the linked object, can we link to just the timestamps (or data) of the corresponding object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean instead of displaying the neurodata_type, show just the timestamps/data of the corresponding object? Or do you mean changing how the timestamp_link and data_link properties get set up?

Copy link
Contributor

@rly rly Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The former. In practice, we do not show arrays yet in this html repr, so nothing will show up, but it would be less confusing as to why timestamps = another NWB object.

It would however be nice to say that this is a linked object, so maybe we could alter the key to say something like
→ timestamps (link to <path to timestamps of other NWB object -- use container.get_ancestors() and make a nice string path out of it>)"

Copy link
Contributor Author

@stephprince stephprince Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I was just looking at the items in the 'timestamp_link' subheading and didn't realize the timestamps for the other timeseries were still showing the linked object. Yes, I can try to change that as well as alter the key to indicate it is a linked object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. Yes I was looking at the timestamps field and not the timestamps_link field.


return super()._generate_field_html(key, value, level, access_code)

Check warning on line 295 in src/pynwb/base.py

View check run for this annotation

Codecov / codecov/patch

src/pynwb/base.py#L295 

Added line #L295 was not covered by tests

@property
def time_unit(self):
return self.__time_unit
Expand Down