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

Multichannel Timeseries #418

Merged
merged 90 commits into from
Nov 20, 2024
Merged

Multichannel Timeseries #418

merged 90 commits into from
Nov 20, 2024

Conversation

droumis
Copy link
Contributor

@droumis droumis commented Aug 29, 2024

image

@droumis droumis changed the title Multichannel Timeseries Example [WIP] Multichannel Timeseries Example Aug 29, 2024
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

1 similar comment
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

@droumis
Copy link
Contributor Author

droumis commented Sep 27, 2024

Just noting that for the multichan.ipynb, I'm having to use an Overlay instead of e.g. curves_overlay = hv.NdOverlay(curves, 'Channel', sort=False)... I wonder if this impacts performance at all

@droumis
Copy link
Contributor Author

droumis commented Sep 27, 2024

Note: Check to ensure that the resultant downsampled data pyramid doesn't ever live entirely in memory prior to getting written to disk.

@droumis
Copy link
Contributor Author

droumis commented Sep 27, 2024

Note: find better principled way to determine factors for creating the data pyramids..
The following doesn't work as the number of channels scales
FACTORS = list(np.array([1, 2, 4, 8, 16, 32, 64, 128, 256]) ** (len(ts_ds["channel"]) // 4))

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review
the pages of the projects you touched before merging this PR: https://holoviz-dev.github.io/examples/.
You can also download an archive of the site from the workflow summary page which comes in handy
when your dev site built was overriden by another PR (we have a single dev site!).

@droumis droumis marked this pull request as ready for review November 7, 2024 15:43
@droumis droumis changed the title [WIP] Multichannel Timeseries Example Multichannel Timeseries Example Nov 7, 2024
Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Comment on lines +408 to +409
" print(f'Pyramid Already Exists: {PYRAMID_PATH}')"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the bottom of this cell is a good place to add f.close().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f is only used in conversion to xarray. I added a context manager instead of f.close()

Copy link
Contributor

Choose a reason for hiding this comment

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

Arf now I get this error when the pyramid gets created, while I don't when close is called afterwards. I'll make the change.

OSError: Can't synchronously read data (dset_id is not a dataset ID)

multichannel_timeseries/large_multichan.ipynb Outdated Show resolved Hide resolved
Copy link
Contributor

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

There are still some unresolved comments, specially for the large_multichan notebook.

@droumis droumis requested a review from maximlt November 19, 2024 23:57
@droumis
Copy link
Contributor Author

droumis commented Nov 19, 2024

@maximlt, I think I've addressed everything identified up until now. Sorry for not seeing some of the previous review comments until recently. Thanks for the epic review!

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

Copy link
Contributor

Your changes were successfully integrated in the dev site, make sure to review the pages of the projects you touched before merging this PR.

@maximlt
Copy link
Contributor

maximlt commented Nov 20, 2024

Thanks for the epic review!

We got well over-trained to do this last year with the NF SDF grant! Thanks for adjusting the example based on our feedback, it's such a cool one. It should totally be in the Featured section, except that I think keeping 8 examples there sounds reasonable and I didn't which one to remove, so that's for later :) Merging, congrats!

@maximlt maximlt merged commit 6f0b2d7 into main Nov 20, 2024
9 checks passed
@jbednar
Copy link
Contributor

jbednar commented Nov 20, 2024

I agree that 8 is the max for that section. It's a very tough call; those are all very cool examples, but if I think about what they demonstrate, this example covers things not included there at all, so I do think it should bump something out. I'd guess we should replace Exoplanets in that group, given that it's not currently showing any plots (which is maybe an error?).

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.

3 participants