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

Seg Array Optimization #208

Merged
merged 48 commits into from Apr 2, 2023
Merged

Seg Array Optimization #208

merged 48 commits into from Apr 2, 2023

Conversation

CPBridge
Copy link
Collaborator

@CPBridge CPBridge commented Dec 5, 2022

A few improvements focused on improving the efficiency of building segmentation arrays from loaded Segmentation objects.

This was achieved through:

  • Re-ordering loops in the construction of the seg_frames_matrix to avoid searching through the large _frames_lut more than necessary.
  • Creating a specialized case for the combine_segments option, rather than implementing it as a postprocessing of the standard routine. This reduces memory consumption significantly as it is now O(1) rather than O(n) in the number of segments requested.
  • Being more careful with dtypes, and allowing the user to specify a dtype
  • Adding an option to skip checks for overlapping segments (skip_overlap_checks), which results in a further modest speed-up.

@pieper you should now find that your example from #202 runs significantly faster and uses much less memory than before when combine_segments=True (especially in the case where the full set of segments is requested).

I went a little way down the path of JITing the methods with numba, but it actually slowed things down. There may be a bit more performance improvement possibly using threading, but we are hitting on some numpy limitations

@CPBridge CPBridge added the enhancement New feature or request label Dec 5, 2022
@pieper
Copy link
Member

pieper commented Dec 5, 2022

Thanks @CPBridge ! Let me know if you need me to test before you merge or i can just wait until @hackermd has had a look through it.

@CPBridge
Copy link
Collaborator Author

CPBridge commented Dec 5, 2022

Thanks Steve. If you do have time to test on your hardware at some point during the process that would be very helpful. I think before or after @hackermd has a look would be fine, I don't expect the core numerical process to change much

@CPBridge
Copy link
Collaborator Author

CPBridge commented Dec 5, 2022

A couple of other points to note:

  • It will still be slow to load the file initially, that will require a fix in pydicom.
  • Please try with and without the skip_overlap_checks option and see how this affects things

@hackermd
Copy link
Collaborator

hackermd commented Dec 5, 2022

Thanks @CPBridge. I will take a look.

It will still be slow to load the file initially, that will require a fix in pydicom

See pydicom/pydicom#1734

src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
@hackermd hackermd self-requested a review December 6, 2022 01:26
Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

A few suggestions regarding style.

@CPBridge
Copy link
Collaborator Author

@hackermd I have refactored out all the database logic and I think this is ready to go

@CPBridge CPBridge requested a review from hackermd March 11, 2023 19:48
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

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

@hackermd I have addressed the various comments, mostly just accepting all suggestions. I have attempted to improve the docstrings for the _SegDBManager class

docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
@CPBridge CPBridge requested a review from hackermd April 1, 2023 20:45
@CPBridge CPBridge merged commit dc240f6 into master Apr 2, 2023
10 checks passed
@hackermd hackermd deleted the seg_array_optimization branch April 2, 2023 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants