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

Add docs for using the family file driver with PyNWB #1949

Merged
merged 18 commits into from
Aug 20, 2024

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Aug 19, 2024

Motivation

Fix #1948

Add docs for using the family file driver with PyNWB

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running ruff check . && codespell from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.20%. Comparing base (3792136) to head (b5d7b01).
Report is 21 commits behind head on dev.

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1949   +/-   ##
=======================================
  Coverage   92.20%   92.20%           
=======================================
  Files          27       27           
  Lines        2656     2656           
  Branches      693      693           
=======================================
  Hits         2449     2449           
  Misses        134      134           
  Partials       73       73           
Flag Coverage Δ
integration 73.15% <ø> (ø)
unit 83.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@cboulay
Copy link

cboulay commented Aug 19, 2024

Thank you very much for putting this together!

I think any realistic scenario where the user has on disk file size constraints would probably also have in-memory constraints, so it's probably rare that they'd have their entire dataset in memory. I think the provided example would be more practical and more illustrative if the data argument was a H5DataIO object with maxshape=(None, nchan).

However, I recognize that this might complicate the message so perhaps it would be enough to just write as much in words.

File splitting is most useful with large data and is likely best combined with Iterative Writing. The data object in this example can be replaced with DataChunkIterator or H5DataIO. See Iterative Writing documentation for more information.

This glosses over the fact that some of these operations would be done in a different order in an iterative writing scenario but I think that's OK as long as the Iterative Writing documentation is instructive enough.

@oruebel
Copy link
Contributor Author

oruebel commented Aug 20, 2024

I think the provided example would be more practical and more illustrative if the data argument was a H5DataIO object with maxshape=(None, nchan).

@cboulay I updated the example to include this and then also write data iteratively to that dataset. Can you please take a look and see if that addresses your comment.

@cboulay
Copy link

cboulay commented Aug 20, 2024

There are just a couple small grammar errors noted by me and Ben but otherwise I'd say it's super clear and exactly what I was looking for. Thanks @oruebel!

Co-authored-by: Ben Dichter <[email protected]>
@oruebel
Copy link
Contributor Author

oruebel commented Aug 20, 2024

@stephprince ruff seems to fail due to files in docs/notebooks that have not been changed in this PR so this may be a separate issue.

Other than the issue with ruff this PR is ready for review.

Copy link
Contributor

@stephprince stephprince left a comment

Choose a reason for hiding this comment

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

Looks good and read clearly! I added some small formatting suggestions, mainly some code references that were italicized that I think were meant to be code blocks. If the italics were intentional you can just ignore those suggestions.

It looks like some of the ruff linter settings were deprecated - I will address those in a separate PR and we can ignore it here for now.

@oruebel
Copy link
Contributor Author

oruebel commented Aug 20, 2024

I added some small formatting suggestions, mainly some code references that were italicized that I think were meant to be code blocks. If the italics were intentional you can just ignore those suggestions.

Thanks for the suggestions. All those edits looked good and I committed them to this PR.

@oruebel
Copy link
Contributor Author

oruebel commented Aug 20, 2024

It looks like some of the ruff linter settings were deprecated - I will address those in a separate PR and we can ignore it here for now.

Sounds good. I think this PR is then ready to be merged.

@oruebel oruebel merged commit 12598ea into dev Aug 20, 2024
23 of 24 checks passed
@oruebel oruebel deleted the add/family_driver_docs branch August 20, 2024 19:05
@oruebel oruebel mentioned this pull request Aug 20, 2024
6 tasks
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.

[Documentation]: Add example for using family driver to modular stroage docs
4 participants