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

Make vtk contour take an affine #1165

Merged
merged 20 commits into from Dec 4, 2017

Conversation

Projects
6 participants
@kesshijordan
Contributor

kesshijordan commented Dec 17, 2016

I took the code from the contour function from fvtk.py (which doesn't use an affine) and combined it with code from the slicer in actor.py to make a function contour_actor in actor.py that uses an affine to place contours. I'm using this to display a roi with background image (slicer) and streamlines. I haven't written tests, yet.

This replaces #1163 so that the PR is only one feature. Thanks for the git assist, @akeshavan

@arokem

This comment has been minimized.

Member

arokem commented Dec 17, 2016

LGTM. Might need a test to keep up the test-coverage in here. Maybe @Garyfallidis can give some guidance on how to write tests for this kind of thing.

@codecov-io

This comment has been minimized.

codecov-io commented Dec 17, 2016

Codecov Report

Merging #1165 into master will increase coverage by 0.03%.
The diff coverage is 83.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
+ Coverage   87.03%   87.07%   +0.03%     
==========================================
  Files         228      227       -1     
  Lines       29034    29060      +26     
  Branches     3128     3123       -5     
==========================================
+ Hits        25269    25303      +34     
+ Misses       3058     3047      -11     
- Partials      707      710       +3
Impacted Files Coverage Δ
dipy/direction/peaks.py 79.32% <ø> (ø) ⬆️
dipy/utils/optpkg.py 71.42% <ø> (ø) ⬆️
dipy/viz/actor.py 79.76% <75.43%> (+0.21%) ⬆️
dipy/viz/tests/test_actors.py 71.01% <90.62%> (+6.08%) ⬆️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️
dipy/viz/window.py 64.22% <0%> (-0.64%) ⬇️
dipy/viz/ui.py 89.86% <0%> (-0.38%) ⬇️
dipy/viz/fvtk.py 61.36% <0%> (-0.29%) ⬇️
dipy/tracking/local/__init__.py 100% <0%> (ø) ⬆️
dipy/viz/tests/test_ui.py 83.53% <0%> (ø) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0964732...2750fa9. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Dec 17, 2016

Coverage Status

Coverage decreased (-0.2%) to 87.776% when pulling 3740094 on kesshijordan:fix_contour3 into 83ebf37 on nipy:master.

@MarcCote

MarcCote requested changes Jan 7, 2017 edited

Thanks for adding this actor :). Tests would be really nice as it would help us to run the code see what kind of contour we should expect.

@@ -201,6 +201,142 @@ def copy(self):
return image_actor
def contour_actor(data, affine=None, levels=[50],
colors=[np.array([1.0, 0.0, 0.0])], opacities=[1]):
"""Take a volume and draw surface contours for any any number of

This comment has been minimized.

@MarcCote

MarcCote Jan 7, 2017

Contributor

Typo "any any".

This comment has been minimized.

@Garyfallidis

Garyfallidis Apr 18, 2017

Member

Hi @kesshijordan, thank you for your PR. Can you add a small tutorial in docs/example? @ranveeraggarwal, @MarcCote can you give advice to @kesshijordan how to write tests for this PR?

This comment has been minimized.

@kesshijordan

kesshijordan Sep 15, 2017

Contributor

Hi, @Garyfallidis. Happy to; thanks for the feedback. I made a small tutorial and wrote a few tests.

If None then the identity matrix is used.
levels : array_like
Sequence of thresholds for the contours taken from image values needs
to be same datatype as `vol`.

This comment has been minimized.

@MarcCote

MarcCote Jan 7, 2017

Contributor

What is vol?

This comment has been minimized.

@Garyfallidis

Garyfallidis Apr 18, 2017

Member

I believe this should be data.

This comment has been minimized.

@kesshijordan
Parameters
----------
data : array, shape (X, Y, Z) or (X, Y, Z, 3)
A grayscale or rgb 4D volume as a numpy array.

This comment has been minimized.

@MarcCote

MarcCote Jan 7, 2017

Contributor

Can you really have a contour plot of an RGB volume? The provided levels thresholds would correspond to what?

This comment has been minimized.

@kesshijordan

kesshijordan Sep 15, 2017

Contributor

moot point now. removed the multiple contours.

levels : array_like
Sequence of thresholds for the contours taken from image values needs
to be same datatype as `vol`.
colors : (N, 3) ndarray

This comment has been minimized.

@MarcCote

MarcCote Jan 7, 2017

Contributor

What does N represent?

This comment has been minimized.

@kesshijordan

kesshijordan Sep 15, 2017

Contributor

changed functionality to only binary (no multiple contours) so this is now 1 x 3

@arokem

This comment has been minimized.

Member

arokem commented Apr 13, 2017

Hey @kesshijordan : have you had a chance to address the comments that @MarcCote made in his review? Any more thoughts on testing (from anyone...)?

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Apr 15, 2017

Hi, @arokem. I have addressed some of them, and am writing tests modeled after the slicer tests, but got a bit stuck on the multi-level contours (the way the original function was written, it puts multiple skins into the assembly, each with their own opacity, color, etc, based on a set of levels that are provided by the user). After testing the multi-level contouring, I'm not sure I understand the original intent of the contour actor; the main functionality I wanted is to make 3D ROI's... instead of making multiple contours we could instead just have it render a single skin ROI... Unless anyone disagrees, I think I'll modify the actor to display a single binary ROI as a volume and get rid of the rest of the contouring?

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Apr 18, 2017

I made a tutorial and wrote a few tests, modeled after the tests written for the slicer actor.

@coveralls

This comment has been minimized.

coveralls commented Apr 18, 2017

Coverage Status

Coverage increased (+0.008%) to 88.012% when pulling f006c13 on kesshijordan:fix_contour3 into 83ebf37 on nipy:master.

@MarcCote

I made a few comments but I'll wait after you fixed the unit test and the example script to continue.

from dipy.tracking.local import ThresholdTissueClassifier
from dipy.tracking import utils
from dipy.tracking.local import LocalTracking
from dipy.viz import fvtk

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2017

Contributor

Avoid using the soon-to-be-deprecated module fvtk, if possible. Instead, use:
from dipy.viz import actor, window

to the rendering.
"""
ren = fvtk.ren()

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2017

Contributor
ren = window.ren()
ren.add(streamlines_actor)
ren.add(seedroi_actor)
from dipy.tracking.local import ThresholdTissueClassifier
from dipy.tracking import utils
from dipy.tracking.local import LocalTracking
from dipy.viz import fvtk

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2017

Contributor

Avoid using the soon-to-be-deprecated module fvtk, if possible. Here you can use:
from dipy.viz import actor, window

data2[35:40, 25, 25] = 1.
affine = np.eye(4)
surface2 = actor.surface_actor(data2, affine,
colors=np.array([0, 1, 1]),

This comment has been minimized.

@MarcCote

MarcCote Apr 21, 2017

Contributor

The test is failing for me with the following error:

TypeError: surface_actor() got an unexpected keyword argument 'colors'
@coveralls

This comment has been minimized.

coveralls commented May 18, 2017

Coverage Status

Coverage decreased (-2.7%) to 85.255% when pulling 653f109 on kesshijordan:fix_contour3 into 83ebf37 on nipy:master.

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented May 18, 2017

Thanks for the comments, @MarcCote! Sorry about the delay!

@nipy nipy deleted a comment from coveralls Jul 3, 2017

@coveralls

This comment has been minimized.

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 84.991% when pulling 5f678a9 on kesshijordan:fix_contour3 into e498e68 on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 15, 2017

Coverage Status

Coverage decreased (-0.3%) to 84.991% when pulling 5f678a9 on kesshijordan:fix_contour3 into e498e68 on nipy:master.

@skoudoro skoudoro referenced this pull request Oct 4, 2017

Merged

Replacing fvtk by the new viz API #1347

2 of 2 tasks complete
@MarcCote

Hey @kesshijordan, thanks for the modification. I tried it and it works great. I've made a few more comments.

Regarding the unit test, I do think it can run on Travis with the right setting for the snapshot function. At least, it does on my computer with VTK5.8 and XVFB.

data2[35:40, 25, 25] = 1.
affine = np.eye(4)
surface2 = actor.surface_actor(data2, affine,
color=np.array([0, 1, 1]),

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

PEP8: Wrong alignment.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed... I added pep8 to my editor so this should be better in the future : )

csa_model = CsaOdfModel(gtab, sh_order=6)
csa_peaks = peaks_from_model(csa_model, data, default_sphere,
relative_peak_threshold=.8,

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

PEP8: Wrong alignment.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

# Initialization of LocalTracking. The computation happens in the next step.
streamlines = LocalTracking(csa_peaks, classifier, seeds, affine,
step_size=2)

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

PEP8: Wrong alignment.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

surface_color = [0,1,1]
seedroi_actor = actor.surface_actor(seed_mask, affine,
surface_color, surface_opacity)

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

PEP8: Wrong alignment.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

streamlines = list(streamlines)
"""
We will create a streamline actor from the streamlines

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

Missing a .

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

#window.show(renderer2)
arr = window.snapshot(renderer, 'test_surface.png', offscreen=False)
arr2 = window.snapshot(renderer2, 'test_surface2.png', offscreen=False)

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

Since our tests on Travis uses XVFB (virtual framebuffer) we need to render the scene offscreen. Use offscreen=True instead.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

@@ -159,6 +162,112 @@ def test_slicer():
npt.assert_array_equal([1, 3, 2] * np.array(data.shape),
np.array(slicer.shape))
@npt.dec.skipif(skip_surface)

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

There is no reason to skip this test.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

else:
skip_slicer = False
skip_surface = False

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

Using the option offscreen=True with window.snapshot, there is no reason to skip this test.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

else:
skip_slicer = False
skip_surface = False

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

Using the option offscreen=True with window.snapshot, there is no reason to skip this test.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

@@ -26,10 +26,13 @@
if actor.have_vtk:
if actor.major_version == 5 and use_xvfb:
skip_slicer = True
skip_surface = True

This comment has been minimized.

@MarcCote

MarcCote Oct 7, 2017

Contributor

Using the option offscreen=True with window.snapshot, there is no reason to skip this test.

This comment has been minimized.

@kesshijordan

kesshijordan Oct 13, 2017

Contributor

fixed

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Oct 13, 2017

Thanks for the review, @MarcCote!! I addressed all of your comments and pushed the changes.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 14, 2017

@Garyfallidis this looks good to me. I'll let you merge it once you have a final look at it.

@dmreagan dmreagan added this to UI PR needs a review in Viz Module Dec 1, 2017

@@ -208,6 +207,125 @@ def copy(self):
return image_actor
def surface_actor(data, affine=None,

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2017

Member

Rename to contour_from_roi

image_resliced.SetInterpolationModeToLinear()
image_resliced.Update()
# code from fvtk contour function

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2017

Member

remove comment and reduce empty lines

report = window.analyze_snapshot(arr, find_objects=True)
npt.assert_equal(report.objects, 1)
npt.assert_array_equal([1, 3, 2] * np.array(data.shape),
np.array(slicer.shape))
@npt.dec.skipif(not run_test)
@xvfb_it
def test_surface():

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2017

Member

test contour_from_roi()

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2017

Member

surface_actor is too generic, sorry...

@@ -0,0 +1,99 @@
"""
==================================
Visualization of 3D Surface Rendered ROI with Streamlines

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 1, 2017

Member

I would suggest to change the title to
Visualization of ROI contour or surface..

This comment has been minimized.

@kesshijordan

kesshijordan Dec 1, 2017

Contributor

"Visualization of ROI Surface Rendered with Streamlines"

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 1, 2017

@kesshijordan I have some final minor changes. After you make the changes will be happy to merge! Thx! :)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 1, 2017

Finally, ... please remove WIP from the title of this PR and connect your tutorial to examples_index.rst and valid_examples.txt. Thanks again!!! :)

@kesshijordan kesshijordan changed the title from WIP: Make vtk contour take an affine to Make vtk contour take an affine Dec 1, 2017

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 3, 2017

Cool, very well done @kesshijordan . One last thing rename the filename from viz_surface_actor.py to viz_contour_from_roi.py or viz_roi_contour.py and after that will merge! :)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Dec 4, 2017

Great! Thank you @kesshijordan ! Merged! 👍

@Garyfallidis Garyfallidis merged commit ae69540 into nipy:master Dec 4, 2017

2 of 3 checks passed

codecov/patch 83.47% of diff hit (target 87.03%)
Details
codecov/project 87.07% (+0.03%) compared to 0964732
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Viz Module automation moved this from UI PR needs a review to Done Dec 4, 2017

@kesshijordan

This comment has been minimized.

Contributor

kesshijordan commented Dec 4, 2017

Thanks, @Garyfallidis !!!

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment