Skip to content
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

ENH: Add crosshairs, modify mode #5

Merged
merged 22 commits into from
Nov 20, 2014

Conversation

larsoner
Copy link

Ready for review/merge. Works well on my system. Feel free to tweak the appearance.

@@ -58,6 +58,7 @@ dist/
.shelf
.tox/
.coverage
cover/
Copy link
Author

Choose a reason for hiding this comment

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

for when nosetests is run with --cover-html

@larsoner
Copy link
Author

BTW it looks like using pip to install scipy is really slow. On my other repos I use anaconda to configure my travis runs. I can switch nibabel over to that system if you want. I think what sklearn does, too.

@matthew-brett
Copy link
Owner

Ah - no - I think skimage switched back to using wheels recently - see :
https://github.com/scikit-image/scikit-image/blob/master/tools/travis_setup.sh

If you'd consider doing that, it would be great to update the nibabel build
to do that too...

@larsoner
Copy link
Author

Unfortunately I have never worked with wheels before. For this PR I'll just remove the scipy pip install (since it's not needed anyway) and we can do wheels in a subsequent PR. Sound good?

@larsoner
Copy link
Author

BTW I was actually talking about sklearn (which appears to still use conda), not scikit-image (which does appear to use wheels):

https://github.com/scikit-learn/scikit-learn

Would you recommend I try to follow the scikit-image practices for setting up the wheels? I'm not sure how to do it but I can read tutorials and check out their code.

@matthew-brett
Copy link
Owner

Yeah - it's just:

pip install --no-index -f http://travis-wheels.scikit-image.org scipy
cython whatever

@larsoner
Copy link
Author

Should I use the scikit-image wheels, or should I set up new wheels for nibabel?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 3df0d6f on Eric89GXL:pauls-viewer-tweaks into efcbd55 on matthew-brett:pauls-viewer.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.22%) when pulling a704024 on Eric89GXL:pauls-viewer-tweaks into efcbd55 on matthew-brett:pauls-viewer.

@larsoner
Copy link
Author

It appears to work by going through the scikit-image wheels. Got a nice coverage increase, too :)

While I'm digging around in there, do we really need 3.2, 3.3, and 3.4 builds? Maybe just 3.4 (skip 3.2 and 3.4) or 3.2 and 3.4 (skip 3.3) would be enough so that we can save on CI time.

@larsoner larsoner force-pushed the pauls-viewer-tweaks branch 5 times, most recently from b493001 to 4381c75 Compare October 20, 2014 02:59
@coveralls
Copy link

Coverage Status

Coverage increased (+1.23%) when pulling 4381c75 on Eric89GXL:pauls-viewer-tweaks into efcbd55 on matthew-brett:pauls-viewer.

@larsoner larsoner force-pushed the pauls-viewer-tweaks branch 2 times, most recently from 4b5ed71 to 3b17203 Compare October 20, 2014 03:12
@matthew-brett
Copy link
Owner

Looks very nice.

I notice that if you close one of the linked windows, then the other crashes in self.callbacks.process - do you think it would be easy to get round that?

@larsoner
Copy link
Author

Yeah, it can be fixed by putting a call to unlinking in an on_close handler. Want me to take care of it?

@matthew-brett
Copy link
Owner

Thanks - that would be very helpful.

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

I couldn't actually reproduce the bug. I used this code:

import matplotlib.pyplot as plt
plt.ion()
import nibabel as nib
viewer1 = nib.load('mri.nii').plot()
viewer2 = nib.load('fmri.nii').plot()
viewer1.link_to(viewer2)

And I tried closing the figures by hitting escape, by pressing the X in the window decorator, and by doing viewer1.close() and none of them triggered an error. I added the handler anyway just to be safe. It it still doesn't work on your end, I'll need to know more specifics about how you produced the problem. Maybe I'm using a newer version of matplotlib that protects against bad callbacks or something...

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) when pulling 263820c on Eric89GXL:pauls-viewer-tweaks into efcbd55 on matthew-brett:pauls-viewer.

@matthew-brett
Copy link
Owner

No, same crash - I'm on OSX - matplotlib 1.4.2, tk backend.

What do you get for this:

import os
from os.path import join as pjoin
import matplotlib.pyplot as plt
import nibabel as nib
data_path = os.getcwd()
viewer1 = nib.load(pjoin('highres001.nii')).plot()
viewer2 = nib.load(pjoin('bold.nii')).plot()
viewer1.link_to(viewer2)
viewer1.close()
plt.show()

?

@matthew-brett
Copy link
Owner

If I comment out the viewer1.close() above, then close the anatomical viewer, and then click inside the EPI viewer, I get this

Exception in Tkinter callback
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-tk/Tkinter.py", line 1486, in __call__
    return self.func(*args)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/backends/backend_tkagg.py", line 398, in button_press_event
    FigureCanvasBase.button_press_event(self, x, y, num, dblclick=dblclick, guiEvent=event)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/backend_bases.py", line 1870, in button_press_event
    self.callbacks.process(s, mouseevent)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/cbook.py", line 540, in process
    proxy(*args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/cbook.py", line 415, in __call__
    return mtd(*args, **kwargs)
  File "/Users/mb312/usr/local/lib/python2.7/site-packages/nibabel/viewers.py", line 416, in _on_mouse
    self._set_position(*np.dot(self._affine, idxs)[:3])
  File "/Users/mb312/usr/local/lib/python2.7/site-packages/nibabel/viewers.py", line 361, in _set_position
    self._notify_links()
  File "/Users/mb312/usr/local/lib/python2.7/site-packages/nibabel/viewers.py", line 262, in _notify_links
    link().set_position(*self.position[:3])
  File "/Users/mb312/usr/local/lib/python2.7/site-packages/nibabel/viewers.py", line 277, in set_position
    self._draw()
  File "/Users/mb312/usr/local/lib/python2.7/site-packages/nibabel/viewers.py", line 431, in _draw
    ax.figure.canvas.blit(ax.bbox)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/backends/backend_tkagg.py", line 354, in blit
    tkagg.blit(self._tkphoto, self.renderer._renderer, bbox=bbox, colormode=2)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/matplotlib/backends/tkagg.py", line 24, in blit
    tk.call("PyAggImagePhoto", photoimage, id(aggimage), colormode, id(bbox_array))
TclError: this isn't a Tk application

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

Okay, I cleaned up the logic a little bit. While I was in there I also added a nicer __repr__ of the OrthoSlicer3D class. Hopefully it works for you now. I might have time to try on OSX later this weekend or early next week, but I don't have the problem on Windows or Linux where I have been testing. I think those both use the PyQt backend, which I guess could matter.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) when pulling dfb82b5 on Eric89GXL:pauls-viewer-tweaks into efcbd55 on matthew-brett:pauls-viewer.

@matthew-brett
Copy link
Owner

Eric - I'd like to merge these changes and work on the code a bit. I think there's a problem if the second image has a different (sagittal coronal etc) orientation from the first. OK with you? Or do you have other changes you're in the middle of?

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

Weird. I tried it with your images, and also with some that had a different (non-RAS) orientation and they all seemed okay. The idea is that the position coordinates should always be X, Y, Z, and the affine should do the translation into the appropriate indices (with the necessary [::1] and .Tcalls). Yep, feel free to merge and work on it. As with the parrect code, I'll be interested to see what fixes are necessary. Still learning this MRI stuff.

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

...there also very well already be some code for e.g. extracting a particular coronal slice so you don't have to manually keep track of the flips and order, but I wasn't familiar enough with the code base to know. So if you have more efficient / DRY ways of doing things, feel free. The important thing I was shooting for was always displaying the data in the standard orientations (a la freeview), regardless of the order of the data array itself. Same thing with the set_position command. Sounds like I might not have gotten it 100% correct, but it was what I was shooting for :)

@matthew-brett
Copy link
Owner

There is the code as_closest_canonical in nibabel.funcs which flips the image orientation to be closest to RAS. I was thinking of generalizing that routine so you could flip the image orientation to be any arbitrary orientation.

Then the current processing PR adds a function resample_from_to which can resample any image to the space of another image. Is that useful?

For the orientations thing, what do you get doing linked views of these two images?

http://psydata.ovgu.de/philips_achieva_testfiles/conversion2/Phantom_EPI_3mm_sag_15AP_SENSE_13_1.nii

http://psydata.ovgu.de/philips_achieva_testfiles/conversion2/Phantom_EPI_3mm_cor_SENSE_8_1.nii

Is it what you expect?

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

Ahh, as_closest_canonical could thus be used to simplify the slice extraction routines. We could also add a command-line option resample=False to resample to standard RAS space in conjunction with your other PR. freeview has something like this, and it would be cool, although I think starting out with just as_closest_canonical would be a good start.

I don't get the same orientations shown when plotting those two images -- the center of one is not the center of the other. Should they be the same? Equivalently, are the affines set up to make them have the same origin and orientation?

@matthew-brett
Copy link
Owner

Yes - good point - the affines are correct - you can check them by resampling one to the other and viewing the first image and the resampled image (that's what I did just now).

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

You want to try making your simplifications and modifications? I suspect that using the as_closest_canonical will fix the problem.

@matthew-brett
Copy link
Owner

I can do that. I'll have to think about orientations. My gut feeling is that it's best to default to resampling one image onto the other by default, and if not doing that, then leave it as the original orientation.

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

Is the need to resample because one could often be tilted e.g. along every axis relative to another, making the slice matches crap? I had assumed that matching the position, and pulling the corresponding slices directly from each data structure might be "close enough", but I don't know how often affines are skewed relative to one another.

@matthew-brett
Copy link
Owner

Yes, if the images don't have the same orientation already, then there is a good chance they will be tilted relative to each other. For example, I guess it's more common than not to have off-axial EPIs, and the structurals often have coronal or sagittal acquisitions (before they get reoriented for distribution).

We could add 'as_closest_canonical' as one option where resampling is the default, I guess.

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

I'm fine with adding resampling to standard RAS by default, my only concerns would be 1) any data quality degradation and 2) increased processing time. Do you think those problems will be minimal? Either way I think that resample=False (whether default or not) should just mean it uses as_closest_canonical, which is what I meant for my code to achieve anyway.

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

... and if we want to get fancy about it, if one or both of two linked images doesn

@larsoner
Copy link
Author

larsoner commented Nov 1, 2014

(whoops) doesn't have resample=True enabled and the affines don't match, then we can throw a warning about possible data slice orientation issues.

@matthew-brett
Copy link
Owner

Eric - thinking a bit more, I am drawn towards defering this PR and the resampling etc PR until after the release, and committing myself to doing another release in say 4 weeks time.

I want to think a little bit more about the API (or otherwise) for axis names, and let the design of the viewer settle a little out of the light of a release.

Is that OK with you? Are there any other PARREC changes you need?

@larsoner
Copy link
Author

larsoner commented Nov 4, 2014

Sure, that way the more advanced master users can actually get a bit of use on it before releasing to the general public.

No PARREC changes at my end, things work well on the test cases I've run.

@larsoner
Copy link
Author

larsoner commented Nov 4, 2014

But if you're going to base your work off of mine, this PR could be merged into your branch, but not into upstream/master.

@matthew-brett matthew-brett merged commit dfb82b5 into matthew-brett:pauls-viewer Nov 20, 2014
@TheChymera
Copy link

Hi guys, any updates on this? I would really like to use a python NIfTI viewer.

I am using a fresh install of live nipy and

import os
from os.path import join as pjoin
import matplotlib.pyplot as plt
import nibabel as nib
data_path = os.getcwd()
viewer1 = nib.load(pjoin('highres001.nii')).plot()
viewer2 = nib.load(pjoin('bold.nii')).plot()
viewer1.link_to(viewer2)
viewer1.close()
plt.show()

fails at viewer1 = nib.load(pjoin('highres001.nii')).plot() because apparently there is no plot() implemented for NIfTI objects.

matthew-brett pushed a commit that referenced this pull request Mar 14, 2016
FIX: allow applies_to_matrix_dimension to be a comma-seperate list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants