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

Slicer fix #944

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
7 participants
@gauvinalexandre
Copy link
Contributor

gauvinalexandre commented Feb 28, 2016

I discovered a major contradiction and bug in the value scaling and lookup table making of the slicer feature. I found the way to make it work. Therefore, the behavior of the slicer changed and the test_fvtk_actors fails, probably with reason. It has to be adapted. I'll need help, since I don't know how to interpret the error. Also, there were some mistakes in the doc-strings. I took the occasion to clean the code. I've tested the Slicer with VTK 5 and 6 the best I could and ran the fvtk_actor test. Everything looks good.

@@ -38,7 +38,8 @@ def slicer(data, affine=None, value_range=None, opacity=1.,
opacity : float
Opacity of 0 means completely transparent and 1 completely visible.
lookup_colormap : vtkLookupTable
If None (default) then a grayscale map is created.
If None (default) then a grayscale map is created. It doesn't apply

This comment has been minimized.

@MarcCote

MarcCote Feb 29, 2016

Contributor

What do you think of: "This parameter is ignored if data is an RGB volume."

This comment has been minimized.

@gauvinalexandre

gauvinalexandre Feb 29, 2016

Contributor
  • Change doc

    nice ! Very better.

if value_range is None:
value_range = (np.min(data), np.max(data))
else:
# Rescale data

This comment has been minimized.

@MarcCote

MarcCote Feb 29, 2016

Contributor

Isn't it more "clamping" than rescaling?

This comment has been minimized.

@gauvinalexandre

gauvinalexandre Feb 29, 2016

Contributor
  • Change for clamping

Absolutely.

value_range = (np.min(data), np.max(data))
else:
# Rescale data
data[data < value_range[0]] = value_range[0]

This comment has been minimized.

@MarcCote

MarcCote Feb 29, 2016

Contributor

data is going to be modified in-place. This is an unexpected behaviour. If we are going to clamp the data, we should do it on the copy hold by the vtkImageActor.

This comment has been minimized.

@gauvinalexandre

gauvinalexandre Feb 29, 2016

Contributor
  • Independent copy

Yes, you'Re right

value_range = (np.min(data), np.max(data))
else:
# Clamping data to specified value range
data[data < value_range[0]] = value_range[0]

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 4, 2016

Member

Hi @gauvinalexandre and sorry for taking long time to get back to you on this.

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 4, 2016

Member

I think here we have a different understanding of what this function should be doing. I wanted this dataset to interpolate from value_range to (0, 255). I think you want to clamp the data. That is a different goal.

vol = vol.astype('uint8')

# Allocate VTK Image Data
data = np.copy(data)

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 4, 2016

Member

I used vol before so that we don't change the initial data. Or give the impression that we are doing so.

if nb_components == 1:
vol = vol.ravel()
data = data.ravel()

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 4, 2016

Member

I think vol was fine.

if nb_components == 1:
if lookup_colormap is None:
# Create a black/white lookup table.
lut = colormap_lookup_table((0, 255), (0, 0), (0, 0), (0, 1))
lut = colormap_lookup_table((value_range[0], value_range[1]),

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 4, 2016

Member

Ah here you change the range to be anything. Okay this again is a different approach. So, I wonder when you said a bug did really something failed or by reading the docstring you didn't see the purpose of the function? We could add to parameters one for data_range (what I was calling value_range) and one for interp_range what here you use as input to the colormap.

@Garyfallidis

This comment has been minimized.

Copy link
Member

Garyfallidis commented Nov 4, 2016

Here is a first review @gauvinalexandre Let me know what you think. Clamping and interpolation are two different things. We probably should try to support both.

We should also call (optionally) this function for removing outliers. Often most images have only a few outliers with high intensity values and the interpolation generates images with less contrast.

from dipy.segment.threshold import upper_bound_by_percent
@gauvinalexandre

This comment has been minimized.

Copy link
Contributor

gauvinalexandre commented Nov 9, 2016

Hey @Garyfallidis ! This is from a long time ago. I'll get back in my use cases and discuss it with you soon. Thanks for the feedback!

@gauvinalexandre gauvinalexandre force-pushed the gauvinalexandre:slicer_fix branch from 4870b9e to d07d5d1 Nov 28, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 28, 2016

Coverage Status

Coverage decreased (-0.02%) to 87.98% when pulling d07d5d1 on gauvinalexandre:slicer_fix into e87b327 on nipy:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 28, 2016

Current coverage is 85.44% (diff: 33.33%)

Merging #944 into master will decrease coverage by 0.02%

@@             master       #944   diff @@
==========================================
  Files           214        214          
  Lines         24917      24917          
  Methods           0          0          
  Messages          0          0          
  Branches       2526       2526          
==========================================
- Hits          21296      21291     -5   
- Misses         2989       2995     +6   
+ Partials        632        631     -1   

Powered by Codecov. Last update e87b327...d07d5d1

@dmreagan dmreagan added this to UI PR in Progress in Viz Module Dec 1, 2017

@thechargedneutron

This comment has been minimized.

Copy link
Contributor

thechargedneutron commented May 4, 2018

Is this issue still relevant? Ping @dmreagan @ranveeraggarwal

@gauvinalexandre

This comment has been minimized.

Copy link
Contributor

gauvinalexandre commented May 7, 2018

Hi @thechargedneutron ! This issue completely got out of my head a long time ago. What's going on with this repo. I'm not aware of the last year's updates about DIPY. This issue was real. but I didn't took enough time to implement tests and answer to @Garyfallidis review.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Dec 3, 2018

A new visualization tool has been created: FURY - click here to go to the repo.

This PR should moved to the new package. Feel free to reopen it on FURY.

As a reminder, the issue fury-gl/fury#34 has been created.

@skoudoro skoudoro closed this Dec 3, 2018

@skoudoro skoudoro moved this from PR in Progress to Done in Viz Module Dec 17, 2018

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