Skip to content

MRG: Slice viewer #404

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

Merged
merged 24 commits into from
Mar 2, 2016
Merged

MRG: Slice viewer #404

merged 24 commits into from
Mar 2, 2016

Conversation

larsoner
Copy link
Contributor

Let's see if Travis is happy. The rebase wasn't too bad.

@larsoner
Copy link
Contributor Author

The one thing I'm not certain about is this change:

4815e1e#diff-4e1d219713c81bf1e72f89cb1d87d87dR369

I had to make it to comply with newer numpy. I suspect it's the right thing to do, but I'm having a hard time wrapping my head around what the heck the original line was doing by constructing a 6-element list...

@larsoner
Copy link
Contributor Author

Sweet, checks passed.

@larsoner larsoner changed the title ENH: Slice viewer MRG: Slice viewer Feb 11, 2016
@choldgraf
Copy link

hehe, I feel like this guy: http://gph.is/1m6SGhP?tc=1

@ivanov
Copy link
Member

ivanov commented Feb 11, 2016

I think I originally wrote this about 4 years ago! :) 👍

@larsoner
Copy link
Contributor Author

larsoner commented Feb 12, 2016 via email

@choldgraf
Copy link

I believe that this rebase+merge is the code equivalent of picking it up, giving it a quick look-over, blowing on it, and popping it in your mouth :)

@matthew-brett
Copy link
Member

Yummy.

@grlee77
Copy link
Contributor

grlee77 commented Feb 12, 2016

Hi @Eric89GXL. Thanks for renewing the effort to get this into nibabel.

I just made a PR to your branch with a proposed enhancement to allow incrementing/decrementing the volume with the +/- keys. I have found this more convenient than having to mouse over to the fourth axis to drag the volume slider.

I also modified the viewer to raise an immediate TypeError if the input data is complex. This avoids a figure popping up prior to imshow choking on the complex data. (Alternatively, we could raise a warning and auto-convert to magnitude).

@grlee77
Copy link
Contributor

grlee77 commented Feb 12, 2016

One other issue I that could potentially be addressed either here or in a future PR is the scaling of the timecourse plot that appears in the 4th axis for 4D data.

In my experience, the range of the plot does not get adjusted appropriately and the timecourse ends up plotted as a nearly flat line in the display.

@larsoner
Copy link
Contributor Author

@grlee77 IIRC the ylim is set based on the global min and max of the data, and the line reflects the values in the current voxel across volumes. So if the voxel you're on doesn't traverse a large range relative to all the voxels, this is expected behavior (to make all voxels comparable it's good to fix the ylim).

@grlee77
Copy link
Contributor

grlee77 commented Feb 12, 2016

I guess it is a matter of preference, but I think it would be nice to have an option to rescale to the full range for each individual voxel. As is, it seems that the line will nearly always appear flat for fMRI data where the signal changes of interest are only a small percentage of the baseline intensity. The display range will often be set to a very large range because data.min() will often be near or equal to zero due to voxels outside the object.

@larsoner
Copy link
Contributor Author

Let's sort out the right thing to do with the scaling in a subsequent PR

@grlee77
Copy link
Contributor

grlee77 commented Feb 12, 2016

sure, no problem

self.close()
elif event.key in ["=", '+']:
# increment volume index
new_idx = min(self._data_idx[3]+1, self.n_volumes)
Copy link
Member

Choose a reason for hiding this comment

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

Spaces around the -. And on line 454.

@larsoner
Copy link
Contributor Author

Flake errors fixed

@effigies
Copy link
Member

Since Travis didn't run on the latest commit, for some reason, I pushed the branch to my repo: https://travis-ci.org/effigies/nibabel/builds/110833187

Tests passing.

@@ -661,3 +662,20 @@ def __getitem__(self):
raise TypeError("Cannot slice image objects; consider slicing image "
"array data with `img.dataobj[slice]` or "
"`img.get_data()[slice]`")

def plot(self):
Copy link
Member

Choose a reason for hiding this comment

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

plot seems a little generic. How about orthoview?

@larsoner
Copy link
Contributor Author

Comments addressed @matthew-brett.

I would probably have time to be secondary mentor on a project, providing guidance mostly on the plotting end of things with matplotlib. I'm not a guru or anything but I do know enough tricks to help make it work fairly efficiently.

class OrthoSlicer3D(object):
"""Orthogonal-plane slicer.

OrthoSlicer3d expects 3-dimensional data, and by default it creates a
Copy link
Member

Choose a reason for hiding this comment

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

Maybe say 3- or 4-dimensional data here.

@matthew-brett
Copy link
Member

Actually - forget the comment - in it goes, I'll do a follow up PR in a second.

Thanks all for being patient, and thanks Eric for the nice work on this one.

matthew-brett added a commit that referenced this pull request Mar 2, 2016
MRG: Slice viewer

Orthogonal slice viewer for 3D and 4D images.  Based on Paul Ivanov's prototype.   Viewers can be yoked so that crosshairs correspond to matching voxel coordinates according to the image affines.
@matthew-brett matthew-brett merged commit 5a42cb3 into nipy:master Mar 2, 2016
grlee77 pushed a commit to grlee77/nibabel that referenced this pull request Mar 15, 2016
MRG: Slice viewer

Orthogonal slice viewer for 3D and 4D images.  Based on Paul Ivanov's prototype.   Viewers can be yoked so that crosshairs correspond to matching voxel coordinates according to the image affines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants