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.

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

implement MultiTimeseries #178

Merged
merged 2 commits into from
Jan 26, 2022
Merged

implement MultiTimeseries #178

merged 2 commits into from
Jan 26, 2022

Conversation

magland
Copy link
Owner

@magland magland commented Jan 25, 2022

No description provided.

@magland magland requested a review from jsoules January 25, 2022 21:25
@magland
Copy link
Owner Author

magland commented Jan 25, 2022

The goes along with the PR of the same name from spikesortingview: magland/spikesortingview#23

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

All makes sense. I had a question about whether something could be simplified.

Comment on lines +11 to +15
def add_panel(self, figure: Figure, *, relative_height: Union[float, None]=None):
self._panels.append({
'figure': figure,
'relative_height': relative_height
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

So in practice, add_panel is used to add a timeseries (figure) along with its relative height to the list of panels that should be presented together. Then, when we want to render them, we call get_composite_figure which collects the individual figures and labels the collection as a MultiTimeseries, which is what gets displayed on the front-end. Right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's right.

Comment on lines +21 to +29
fig0: Figure = p['figure']
v = {
'type': fig0.data['type'],
'label': fig0.label,
'figureDataSha1': _upload_data_and_return_sha1(fig0.get_serialized_figure_data())
}
if p['relative_height'] is not None:
v['relativeHeight'] = p['relative_height']
panels.append(v)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the distinction here is that each of the Figures in the _panels collection could be a stand-alone time series figure, but when they've been collected as a MultiTimeseries that's a new kind of figure that has special rules on the front-end.

Each v ("view" is the mnemonic here?) is taking a could-have-been-viewed-independently Figure and extracting the type, label, and data, along with possibly a relative height, and then adding them to the panels collection. Understanding the context, this all makes sense.

My only question--is there a benefit to storing the full Figure data in self._panels and then extracting a subset of it in get_composite_figure vs storing only the type, label, sha1, and (optional) relative height to begin with?

Copy link
Owner Author

Choose a reason for hiding this comment

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

My only question--is there a benefit to storing the full Figure data in self._panels and then extracting a subset of it in get_composite_figure vs storing only the type, label, sha1, and (optional) relative height to begin with?

This just provides a simpler interface. You use the same figure object to view a standalone figure as to build a composite figure.

@jsoules
Copy link
Collaborator

jsoules commented Jan 25, 2022

Oh, also, this will need to be moved to comply with the directory structure changes implemented in PR #177 (whether before or after the merge)

@magland magland merged commit 3c9920e into main Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants