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 tutorials to showcase GDS for nifti and dicom images #597

Merged

Conversation

yiheng-wang-nv
Copy link
Contributor

@yiheng-wang-nv yiheng-wang-nv commented Jan 27, 2025

This PR addresses the tutorial contribution mentioned in #580

Signed-off-by: Yiheng Wang <vennw@nvidia.com>
Copy link

copy-pr-bot bot commented Jan 27, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@yiheng-wang-nv
Copy link
Contributor Author

Hi @msarahan , could you help to review this PR? Thanks!

@yiheng-wang-nv yiheng-wang-nv changed the title 580 Add a tutorial to showcase GDS for nifti images Add tutorials to showcase GDS for nifti and dicom images Jan 27, 2025
Signed-off-by: Yiheng Wang <vennw@nvidia.com>
@madsbk madsbk added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jan 27, 2025
@yiheng-wang-nv
Copy link
Contributor Author

HI @madsbk , could you help to review this PR, or may I know who should I invite to review it? Thanks!

@madsbk madsbk changed the base branch from branch-25.02 to branch-25.04 February 17, 2025 09:01
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

I don't know much about nibabel and dicom but the KvikIO usage looks good!

Only suggestion I have is to include some information about the thread pool size. E.g., maybe show how to change the number of threads KvikIO uses: https://docs.rapids.ai/api/kvikio/nightly/runtime_settings/#thread-pool-kvikio-nthreads.
By default, KvikIO only uses 1 thread but often more threads improve performance (16-32 threads seems to be a good option in many cases).

kvikio.defaults.num_threads_reset(nthreads=16)

@madsbk
Copy link
Member

madsbk commented Feb 17, 2025

/ok to test

Signed-off-by: Yiheng Wang <vennw@nvidia.com>
Signed-off-by: Yiheng Wang <vennw@nvidia.com>
@yiheng-wang-nv
Copy link
Contributor Author

I don't know much about nibabel and dicom but the KvikIO usage looks good!

Only suggestion I have is to include some information about the thread pool size. E.g., maybe show how to change the number of threads KvikIO uses: https://docs.rapids.ai/api/kvikio/nightly/runtime_settings/#thread-pool-kvikio-nthreads. By default, KvikIO only uses 1 thread but often more threads improve performance (16-32 threads seems to be a good option in many cases).

kvikio.defaults.num_threads_reset(nthreads=16)

Thanks @madsbk , I added a section and rerun both notebooks

Copy link
Member

@madsbk madsbk 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 to me, thanks @yiheng-wang-nv

@madsbk
Copy link
Member

madsbk commented Feb 18, 2025

/ok to test

@madsbk
Copy link
Member

madsbk commented Feb 18, 2025

@yiheng-wang-nv should we merge?

@yiheng-wang-nv
Copy link
Contributor Author

@yiheng-wang-nv should we merge?

Hi @madsbk , thanks! I'm happy to merge

@madsbk
Copy link
Member

madsbk commented Feb 18, 2025

/ok to test

@madsbk
Copy link
Member

madsbk commented Feb 18, 2025

/merge

@rapids-bot rapids-bot bot merged commit 77da587 into rapidsai:branch-25.04 Feb 18, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants