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

Peak representation improvements #433

Merged
merged 23 commits into from Aug 20, 2021
Merged

Conversation

guaje
Copy link
Contributor

@guaje guaje commented May 15, 2021

This PR is a redesign of the current peak_slicer actor. The design of the actor is heavily inspired in #374.

It includes the following features:

  • A simplified creation method based on the simple geometry of the actor.
  • New colors function.
  • Added a vertex shader function that calculates the color given the orientation.
  • Identification of global opacity.
  • Added Look Up Colormap support.
  • New class with attributes and methods suited for this specific actor.
  • Shaders functions to control the visible peaks. Peaks in a range or peaks in selected slices, as shown in these videos.
Range Planes
peak_range.mp4
peak_planes.mp4

@pep8speaks
Copy link

pep8speaks commented May 15, 2021

Hello @guaje! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-28 15:43:08 UTC

@codecov
Copy link

codecov bot commented May 15, 2021

Codecov Report

Merging #433 (757b400) into master (d498b6d) will increase coverage by 0.53%.
The diff coverage is 94.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   88.32%   88.85%   +0.53%     
==========================================
  Files          31       33       +2     
  Lines        6448     6973     +525     
  Branches      770      822      +52     
==========================================
+ Hits         5695     6196     +501     
- Misses        534      546      +12     
- Partials      219      231      +12     
Impacted Files Coverage Δ
fury/actors/peak.py 90.00% <90.00%> (ø)
fury/actor.py 89.18% <95.45%> (+0.13%) ⬆️
fury/actors/tests/test_peak.py 97.95% <97.95%> (ø)
fury/ui/tests/test_helpers.py 100.00% <0.00%> (ø)
fury/ui/tests/test_core.py 96.51% <0.00%> (+0.38%) ⬆️
fury/ui/containers.py 83.81% <0.00%> (+1.56%) ⬆️
fury/ui/helpers.py 98.21% <0.00%> (+5.35%) ⬆️

@guaje guaje marked this pull request as ready for review May 17, 2021 22:21
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @guaje,

here a quick review. I need to try your PR and go more into details.

Thank you for this feature

fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actors/peak.py Outdated Show resolved Hide resolved
fury/actors/peak.py Outdated Show resolved Hide resolved
fury/actors/peak.py Outdated Show resolved Hide resolved
fury/actors/peak.py Outdated Show resolved Hide resolved
fury/actors/peak.py Outdated Show resolved Hide resolved
@guaje guaje requested a review from skoudoro May 20, 2021 01:02
xyz = np.asarray(center)
else:
xyz = w_pos[idx, :]
valid_peaks = np.nonzero(
Copy link
Contributor Author

@guaje guaje Jul 12, 2021

Choose a reason for hiding this comment

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

@Garyfallidis I ran some profiling tests here and these are the results:

For a 56x72x56 image the operation is adding less than 2 seconds (0:00:01.707887) to the for loop. Also, these are the average time and standard deviation of the operation: Avg = 0:00:00.000017, Std = 0:00:00.000006.

I don't think this is adding a significant delay in the peaks creation, but if you think something else needs to be done, please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

That image is quite small. You should try it with something much larger. Will merge this PR now. But to run the same test with a much larger image. For example, the superresolved HYDI data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran the same test on an HCP 7T subject's peaks with a size of 176x207x173 and these are the results: The operation is adding less than 20 seconds (0:00:19.486101) to the for loop. The average time and standard deviation were similar to the ones obtained with the smaller image.

Additionally, I ran a comparison test between the for loop of the current implementation of the peak slicer and the proposed version and found out that the peaks creation time of the new actor was around 20 minutes (0:03:31.901080) faster than the current version (0:24:02.964906).

@guaje guaje requested a review from Garyfallidis July 14, 2021 21:29
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
@guaje guaje requested a review from skoudoro July 29, 2021 11:23
@Garyfallidis
Copy link
Contributor

Thanks @guaje. Merging.
In some of the docstrings you write (N, 3) array or ndarray. Just say array. Please make that fix in another PR.

@Garyfallidis Garyfallidis merged commit ebe327e into fury-gl:master Aug 20, 2021
@guaje guaje deleted the peak_slicer branch August 23, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants