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

Feat: adding support for sharded precomputed #35

Merged
merged 29 commits into from
Jan 10, 2024
Merged

Conversation

xgui3783
Copy link
Collaborator

@xgui3783 xgui3783 commented Oct 10, 2023

This is a PR adding support for sharded precomputed

This PR should (hopefully) support both read/write from local/http, much like the existing FileAccessor and HttpAccessor.

I will be polishing the PR by:

  • adding test coverage
  • update usage example

in the coming days.

I would like already to gauge if:

  • neuroglancer-scripts is interested to have the ability to read sharded precomputed data source
  • feedbacks on code

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (a6bce3f) 92.61% compared to head (7f1d4b1) 93.96%.

Files Patch % Lines
src/neuroglancer_scripts/sharded_file_accessor.py 98.23% 3 Missing and 2 partials ⚠️
src/neuroglancer_scripts/scripts/scale_stats.py 55.55% 3 Missing and 1 partial ⚠️
src/neuroglancer_scripts/accessor.py 90.90% 1 Missing and 2 partials ⚠️
src/neuroglancer_scripts/sharded_http_accessor.py 95.77% 2 Missing and 1 partial ⚠️
src/neuroglancer_scripts/sharded_base.py 99.17% 1 Missing and 1 partial ⚠️
src/neuroglancer_scripts/volume_reader.py 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   92.61%   93.96%   +1.34%     
==========================================
  Files          25       28       +3     
  Lines        1490     2138     +648     
  Branches      219      308      +89     
==========================================
+ Hits         1380     2009     +629     
- Misses         63       75      +12     
- Partials       47       54       +7     

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

@xgui3783 xgui3783 marked this pull request as ready for review November 2, 2023 09:32
@xgui3783 xgui3783 requested a review from ylep November 6, 2023 13:43
@xgui3783 xgui3783 changed the title Draft: adding support for sharded precomputed Feat: adding support for sharded precomputed Nov 27, 2023
@xgui3783
Copy link
Collaborator Author

@ylep I believe the PR is mostly ready. Can you take a look when you get a chance?

Copy link
Collaborator

@ylep ylep left a comment

Choose a reason for hiding this comment

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

First, I want to thank you a lot @xgui3783 for adding this very significant feature to neuroglancer-scripts! I had been aware of the existence of that sharded storage format, but saw that it was a complex feature to implement so I did not find the time to do so myself.

Next, I apologize for taking so long to review the code. Actually, this is a large amount of code so I have not been able to review it as deeply as I would like. Most of my comments are rather superficial unfortunately; but on the other hand I have no doubt that you have structured the code in a very good way.

My comments are in-line with the code, I'd just like to add one general comment: the new files that you introduced, and those that you contributed to substantially, should have a short copyright header added / updated, in the style of:
https://github.com/HumanBrainProject/neuroglancer-scripts/blob/53e8042411cd6c52d80159f9f8a486e09c877ed1/src/neuroglancer_scripts/chunk_encoding.py#L1C1-L4C74

In particular, the files that contain third-party code should have the copyright and licence of that code included.

With these few minor changes addressed, I will be happy to approve and merge this PR, and make a new release of neuroglancer-scripts!

docs/examples.rst Outdated Show resolved Hide resolved
src/neuroglancer_scripts/sharded_file_accessor.py Outdated Show resolved Hide resolved
src/neuroglancer_scripts/accessor.py Outdated Show resolved Hide resolved
examples/Waxholm/WHS_SD_rat_T2star_v1.nii.gz Outdated Show resolved Hide resolved
script_tests/test_scripts.py Outdated Show resolved Hide resolved
docs/examples.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
@xgui3783
Copy link
Collaborator Author

@ylep I have added an additional commit, which hopefully address most if not all of your concerns.

can you take another look when you get a chance?

Copy link
Collaborator

@ylep ylep left a comment

Choose a reason for hiding this comment

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

@xgui3783 Huge thanks for implementing this feature, it looks to me in a very good state now!

This feature alone is probably worth a 1.2.0 release, don't you think?

@ylep ylep merged commit 0205370 into master Jan 10, 2024
14 checks passed
@xgui3783 xgui3783 deleted the feat_shardedPrecomputed branch January 10, 2024 15:54
@xgui3783
Copy link
Collaborator Author

Thanks @ylep for the quick response. I would fully support a 1.2.0 release.

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