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

Odf slicer #1269

Merged
merged 35 commits into from Jun 22, 2017

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Jun 20, 2017

No description provided.

@arokem

Awesome. A few small comments.

Takes values from 0 (fully transparent) to 1 (opaque)
colormap : None or str
If None then white color is used. Otherwise the name of colormap is
given. Matplotlib colormaps are also supported. Try 'inferno' :).

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Emoticons in the docstring?

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

How about => "Matplotlib colormaps are supported (e.g., 'inferno')"

This comment has been minimized.

@arokem

arokem Jun 21, 2017

Member

This still needs to be changed.

else:
mask = mask.astype(np.bool)

I, J, K = odfs.shape[:3]

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

PEP8: lower-case for these.

Takes values from 0 (fully transparent) to 1 (opaque)
colormap : None or str
If None then white color is used. Otherwise the name of colormap is
given. Matplotlib colormaps are also supported. Try 'inferno' :).

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Same comment as above.

m = odfs[tuple(center.astype(np.int))].copy()

if norm:
m /= abs(m).max()

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Should this be np.abs?

all_faces.append(faces + k * xyz.shape[0])
all_ms.append(m)

# from ipdb import set_trace

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Please remove debugging statement

all_ms.append(m)

# from ipdb import set_trace
# set_trace()

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

And here.

np.reshape(cols, (cols.shape[0] * cols.shape[1],
cols.shape[2])), dtype='f4')
# cols = np.interp(cols, [0, 1], [0, 255]).astype('ubyte')
# vtk_colors = numpy_to_vtk_colors(255 * cols)

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Please remove these commented lines


peak_actor = PeakSlicerActor()

I, J, K = grid_shape

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

PEP8: lower-case

lowercase_cm_name = {'blues': 'Blues', 'accent': 'Accent'}


def create_colormap(v, name='jet', auto=True):

This comment has been minimized.

@arokem

arokem Jun 20, 2017

Member

Can we set the default to plasma?

@Garyfallidis Garyfallidis changed the title from WIP: Odf slicer rebased 2 to WIP: Odf slicer Jun 21, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Jun 21, 2017

Codecov Report

Merging #1269 into master will increase coverage by 0.06%.
The diff coverage is 81.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1269      +/-   ##
==========================================
+ Coverage   87.05%   87.11%   +0.06%     
==========================================
  Files         226      228       +2     
  Lines       28266    28763     +497     
  Branches     3026     3090      +64     
==========================================
+ Hits        24607    25057     +450     
- Misses       2976     3003      +27     
- Partials      683      703      +20
Impacted Files Coverage Δ
dipy/viz/window.py 64.85% <ø> (ø) ⬆️
dipy/viz/tests/test_utils.py 88.09% <ø> (ø)
dipy/viz/tests/test_widgets.py 70.9% <ø> (ø)
dipy/viz/tests/test_window.py 93.54% <ø> (ø)
dipy/viz/fvtk.py 61.64% <100%> (-0.78%) ⬇️
dipy/viz/actor.py 79.55% <76.81%> (+1.37%) ⬆️
dipy/viz/colormap.py 86.42% <80%> (-1.4%) ⬇️
dipy/viz/tests/test_actors.py 64.93% <86.4%> (ø)
dipy/denoise/nlmeans.py 100% <0%> (ø) ⬆️
... and 5 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 09459ff...6cbd867. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 85.279% when pulling 2fc0955 on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 85.279% when pulling 2fc0955 on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage increased (+1.5%) to 87.206% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage increased (+1.5%) to 87.206% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.314% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.314% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.314% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.3%) to 85.314% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage increased (+1.5%) to 87.206% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage increased (+1.5%) to 87.206% when pulling 297800b on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

Garyfallidis added some commits Jun 21, 2017

@Garyfallidis Garyfallidis changed the title from WIP: Odf slicer to Odf slicer Jun 21, 2017

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 21, 2017

@arokem I made all the changes that you requested. @skoudoro or @arokem please merge this PR when travis finishes successfully. Ignore coverage for this PR. Go go team!

@arokem

This comment has been minimized.

Member

arokem commented Jun 21, 2017

I halted all but the last Travis build, in hopes that this will speed things up.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.2%) to 85.416% when pulling 11b3a32 on Garyfallidis:odf_slicer_rebased2 into 09459ff on nipy:master.

@arokem arokem merged commit f934bf0 into nipy:master Jun 22, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 22, 2017

Cool, thanks! Good luck with the seminar at OHBM.

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