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

Slice through ODF fields #1143

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@Garyfallidis
Member

Garyfallidis commented Nov 3, 2016

This PR introduces a new viz.actor (odf_slicer) that allows to slice ODF fields in native or world coordinates matching the underlying anatomy. So, it works nicely with the viz.actor.slicer.

Some of the new features include applying a colormap using all the voxels in the slice or volume.
jet_odfs

Also all the previous capabilities found in sphere_func are available too. Finally, the new function works with memory mapped ODFs and masking which helps reduce memory and video memory usage. I prefer to wait for the UI elements PR (#1140) for creating a tutorial for this function. With the new UI elements we can nicely upgrade some of our reconstruction and viz tutorials. But in the meantime you can review this PR with the tests.

@MarcCote

Overall this looks good to me. Let wait and see what the coverage and the tests will say.
Also, should we rename the test file: test_fvtk_actors.py to test_actor.py?

colormap = get_cmap(newname)
if colormap is None:
e_s = "Colormap '%s' is not yet implemented " % name

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

Should use "Colormap '{}' is not yet implemented.".format(name).

def create_colormap(v, name='jet', auto=True):
"""Create colors from a specific colormap and return it
as an array of shape (N,3) where every row gives the corresponding
r,g,b value. The colormaps we use are similar with those of pylab.

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

pylab -> matplotlib ?

odfs = np.memmap(fname, dtype='float64', mode='w+',
shape=shape)
odfs[:] = 1
# odfs = np.random.rand(10, 10, 10, sphere.vertices.shape[0])

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

Unused

mask=mask, sphere=sphere, scale=.25,
colormap='jet')
fa = 0. * np.random.rand(*odfs.shape[:3])

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

Are we setting the seed for the default random generator somewhere? Otherwise, the tests won't be reproducible.

fa_actor = actor.slicer(fa, affine)
fa_actor.display(None, None, 5)
# renderer.add(fa_actor)

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

Unused

odf_actor.display_extent(0, I, 0, J, k, k + 1)
odf_actor.GetProperty().SetOpacity(1.0)
# window.show(renderer, reset_camera=False)

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

You could check if this test function is being executed in interactive mode or not. To do this, simply add a parameter interactive to the function test_odf_slicer and inside the if __name__ == "__main__": call test_odf_slicer(interactive=True). That way you can execute the test file like this python dipy/viz/tests/test_fvtk_actors.py to start an interatice session and help you while debugging your test.

Have a look at how I did it in https://github.com/nipy/dipy/blob/master/dipy/viz/tests/test_interactor.py#L149 .

report = window.analyze_snapshot(arr, find_objects=True)
npt.assert_equal(report.objects, 11 * 11)
if __name__ == "__main__":
npt.run_module_suite()

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

I don't think this line is needed. Also, if you do decide to implement what I suggested regarding the interactive session debugging, that line would be replaced by calls to all individual tests, e.g. test_odf_slicer(interactive=True).

def odf_slicer(odfs, affine=None, mask=None, sphere=None, scale=2.2,
norm=True, radial_scale=True, opacity=1.,
colormap=None, global_cm=False):
""" Slice spherical fields in native or wold coordinates

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

wold => world

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

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

unused

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.

@MarcCote

MarcCote Nov 3, 2016

Contributor

unused

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 3, 2016

I believe we should have a PR soon to rename all test_fvtk** and a PR to update all tutorials to not be using fvtk. After we get in the UI elements we should do that. Maybe Ranveer can take the lead on that RF. But there is no need to do this RF in this PR. If you agree.

@coveralls

This comment has been minimized.

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.03%) to 88.008% when pulling 24f7e4e on Garyfallidis:odf_slicer into e8eeb70 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 3, 2016

Current coverage is 85.48% (diff: 89.43%)

Merging #1143 into master will increase coverage by 0.03%

@@             master      #1143   diff @@
==========================================
  Files           214        214          
  Lines         24901      25029   +128   
  Methods           0          0          
  Messages          0          0          
  Branches       2524       2539    +15   
==========================================
+ Hits          21277      21395   +118   
- Misses         2993       2997     +4   
- Partials        631        637     +6   

Powered by Codecov. Last update e8eeb70...63bd5f3

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 3, 2016

Should also have a look at #944

@arokem

arokem approved these changes Nov 3, 2016

radial_scale : bool
Scale sphere points according to odf values.
opacity : float
Takes values from 0 (fully transparent) to 1 (non-transparent)

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

"non-transparent" => "opaque"

This comment has been minimized.

@Garyfallidis
lowercase_cm_name = {'blues': 'Blues', 'accent': 'Accent'}
def create_colormap(v, name='jet', auto=True):

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

"jet" is a really bad colormap (see e.g., ). Can we use switch to using a different default (viridis??)? If we want to keep this as a default to support back-compatibility, can we at least add a warning that we will change this in the future?

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

Nope there is no reason to change the default behavior of this function neither to add another warning. What we need to do is use different colormaps in our tutorials. I will add something in the documentation. To suggest using viridis or inferno. Sounds good? By the way I don't think there is one colormap that is good for all the different jobs.

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

I didn't say one colormap is good for all the different jobs you might want to do with it, just that this one is particularly bad for any task, and we shouldn't use it as the default (see: https://jakevdp.github.io/blog/2014/10/16/how-bad-is-your-colormap/). Some visualization software goes so far as to prevent users from using "jet": mwaskom/seaborn@9f5a177

I don't think we need to go that far, but users will tend to stick to the default, so I think we should change it.

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

I am aware of the discussions concerning the jet colormap. Still believe it's a minor issue for dipy but a major issue for matplotlib or seaborn.

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

I respectfully disagree. The colormap is supposed to help derive the information from the data, but this one hides and distorts information.

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

I agree with @arokem. I might add that this is not a minor issue for Dipy.

Choose good defaults, and users will sing the praises of your software and how easy it is to use. Choose poor defaults, and you'll face down user angst over configuration.

I say, let them praise Dipy ;-)

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

Okay, so what should be the default? Ideas?

This comment has been minimized.

@MarcCote

MarcCote Nov 3, 2016

Contributor

If I remembered correctly, matplotlib 2.0 is switching its default to viridis but requires matplotlib 1.5.

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

From top to bottom. Inferno, jet and viridis on a response function. Clearly jet shows the minima better. But the others have a smoother transition.

inferno
jet
viridis

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

And here is hot
hot

opacity : float
Takes values from 0 (fully transparent) to 1 (non-transparent)
colormap : None or str
If None then no color is used.

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

No color, or the default color ("jet"? See comment below)

opacity : float
Takes values from 0 (fully transparent) to 1 (non-transparent)
colormap : None or str
If None then no color is used.

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

No color, or the default colormap?

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

No color for GL means all is white. Will add something.

cells = vtk.vtkCellArray()
cells.SetCells(ncells, all_faces_vtk)
if colormap is not None:

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

I see. It really skips this block if there's no colormap argument. So what does it look like then?

BTW - could you share the code that created the example that you pasted into the PR text? I'd like to try running this on my machine to see that it works well (on Python 3!!)

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 3, 2016

Member

Just take the reconst_csd.py example or even better your SFM example and replace sphere_funcs with odf_slicer. Then to change the view just select the ROI you want with the display_extent method.

Notes
-----
Dipy supports a few colormaps for those who do not use Matplotlib, for
more colormaps consider downloading Matplotlib.

This comment has been minimized.

@arokem

arokem Nov 3, 2016

Member

Maybe a link to the MPL website?

This comment has been minimized.

@Garyfallidis
@arokem

This comment has been minimized.

Member

arokem commented Nov 3, 2016

'viridis' or 'hot'

On Thu, Nov 3, 2016 at 2:44 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

@Garyfallidis commented on this pull request.

In dipy/viz/colormap.py #1143:

@@ -255,3 +261,52 @@ def line_colors(streamlines, cmap='rgb_standard'):
for streamline in streamlines]

 return np.vstack(col_list)

+lowercase_cm_name = {'blues': 'Blues', 'accent': 'Accent'}
+
+
+def create_colormap(v, name='jet', auto=True):

Okay, so what should be the default? Ideas?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1143, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAHPNpZ5_mBcW49LaS2bwl-vXVVcq7dnks5q6lWogaJpZM4KoiF-
.

@arokem

This comment has been minimized.

Member

arokem commented Nov 3, 2016

I'll amend that: I would strongly favor 'viridis' as a default.

It was designed through a careful process that took into account the best
knowledge we have of HCI perceptual considerations.

On Thu, Nov 3, 2016 at 3:32 PM, Ariel Rokem arokem@gmail.com wrote:

'viridis' or 'hot'

On Thu, Nov 3, 2016 at 2:44 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

@Garyfallidis commented on this pull request.

In dipy/viz/colormap.py #1143:

@@ -255,3 +261,52 @@ def line_colors(streamlines, cmap='rgb_standard'):
for streamline in streamlines]

 return np.vstack(col_list)

+lowercase_cm_name = {'blues': 'Blues', 'accent': 'Accent'}
+
+
+def create_colormap(v, name='jet', auto=True):

Okay, so what should be the default? Ideas?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1143, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAHPNpZ5_mBcW49LaS2bwl-vXVVcq7dnks5q6lWogaJpZM4KoiF-
.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 3, 2016

Did they consider spherical 3D functions? Or only images in their evaluation?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 3, 2016

Because they chose one that does not mean we need to choose the same. Here veridis is option 4 for example
https://bids.github.io/colormap/

So, here is magma.
magma

To me veridis looks really depressing. Too green.

Why don't we all investigate a bit which colormap serves dMRI data better? It's good we started this discussion. But we do not need to make a decision immediately. Let's play with different colormaps and see which one prevails.

@coveralls

This comment has been minimized.

coveralls commented Nov 3, 2016

Coverage Status

Coverage increased (+0.05%) to 88.026% when pulling 63bd5f3 on Garyfallidis:odf_slicer into e8eeb70 on nipy:master.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 3, 2016

Plasma looks good too.

image

@arokem

This comment has been minimized.

Member

arokem commented Nov 4, 2016

Yes: viridis, inferno, plasma, and magma are all equally good from a
perceptual/HCI point of view.

I agree that plasma looks good, and has nice cheerful colors. I'd be happy
to replace jet with plasma as a default.

On Thu, Nov 3, 2016 at 4:38 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Plasma looks good too.

[image: image]
https://cloud.githubusercontent.com/assets/134276/19989461/0ac7f9fc-a1fd-11e6-8f28-f40b94bb7280.png


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1143 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAHPNuAwXbUZQ3CA8WjeYpN3ajCenM_vks5q6nB3gaJpZM4KoiF-
.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 4, 2016

Yes, plasma looks great, we can go with that. For fun, can you post one with viridis, I'm curious!
EDIT: Nevermind, I refresh the page and I saw you already posted them :P, thanks.

@arokem

This comment has been minimized.

Member

arokem commented Nov 4, 2016

@MarcCote : there's one with viridis here: #1143 (comment)

Not too bad, in my opinion, but definitely tending towards the green shades

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Nov 4, 2016

On further consideration, the default for the colormap should be set in the create_colormap function and avoid setting it in every function that accepts a colormap in input. Whether or not we should do it in this PR is up to you.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 4, 2016

Hello @arokem and @MarcCote I added plasma as the default for the odf_slicer and added a message in create_colormap suggesting to users to prefer other colormaps. I think that this is better than raising a warning at this stage. This direction gives us time to look into different colormaps but also informs people to start looking and trying alternatives. I hope you are both pleased.

msg = 'Jet is a popular colormap but can often be misleading and'
msg += 'we will remove it from being the default in the near future.'
msg += 'Try for example plasma, viridis, hot or inferno.'
print(msg)

This comment has been minimized.

@arokem

arokem Nov 4, 2016

Member

I think the direction is right. We want to warn people of this now, and deprecate later. You probably want to raise a warning here, rather than just printing a message, though.

This comment has been minimized.

@arokem

arokem Nov 4, 2016

Member

It's a bit strange that the default behavior raises a warning. Why not set the default to be plasma here as well? I guess we do want the default to be consistent across different functions, and you changed it to plasma in several other places.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jan 6, 2017

@Garyfallidis having a closer look it seems that the other test with slicers test_slicer is being skip (because of @npt.dec.skipif(skip_slicer)) on Travis since it is still using VTK5.8.
See https://github.com/nipy/dipy/blob/master/dipy/viz/tests/test_fvtk_actors.py#L25

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 21, 2017

Closing on behalf of #1269

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