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

MNI coordinate calculation, storage, and display #135

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

smerdis
Copy link

@smerdis smerdis commented Nov 3, 2015

This set of changes includes the anat_to_mni() function of the align module, as well as the code to include the calculated MNI coordinates in the CTM file, and JS stuff to load, display, and search this.

…nts in place but is still waiting for WarpMapper and its final form.
…ented anatomicals are saved in the anat_dir, not /tmp, and the commands are printed for debugging use. in future the anatomicals may be saved to /tmp again, since the reoriented versions are not necessary for getting the correct coordinates in the identity transform
…l do, reverted the addition in __init__ also
…commit). reorients anatomicals, does flirt, feeds this to fnirt, saves the resulting warpfield in the database. then samples the warp field to get a Volume of translations, maps this to vertices, and creates arrays corresponding to the pts array but with the MNI coordinates, not the anatomical ones. saves this as a surfinfo in the database.
…an brings up the MNI coords box for the selected vertex. Coordinates are editable, and hitting enter or Submit looks up the nearest MNI coordinate using a kdTree of MNI points which was previously created. It then adds a marker there.
cortex/align.py Outdated
Subject identifier.
xfmname : str
String identifying the transform to be created.
noclean : bool, optional
Copy link

Choose a reason for hiding this comment

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

noclean is not a parameter to this function.

@bcipolli
Copy link

bcipolli commented Nov 7, 2015

This looks interesting; any chance you could post a screen shot of this so I can get a better idea how I might use it?

… are now stored in a per-session tempfile, so they don't permanently clutter the db but are still safe for multiple sessions. some unneeded parameters and imports were removed. additional comments were added, and the do parameter, which allows the function to be run in dummy mode if False.
…te box is left out of the template. I verified that the picker still works in this case.
…NI or magnet isocenter coords, hooked this button up to a function that toggles the coordinate display, made the picker smarter also so that it displays the selected coordinates and not MNI every time.
@smerdis
Copy link
Author

smerdis commented Nov 11, 2015

image

This is a screenshot of the feature in use (latest version, ab216b6). The box in the top left appears when a vertex is selected, and the radio buttons toggle display of MNI and magnet iso-center coords. Clicking outside the brain hides the box again. Coordinates can be entered and searched in the box as well.

…ways searched for nearest mni coordinate. that has been fixed now, meaning one can search by MNI and magnet iso-center coords.
@bcipolli
Copy link

Thank you! I will try this out in the next couple of weeks as I look more deeply into using PyCortex.

@marklescroart
Copy link
Contributor

Howdy,

I've been reviewing the code, and there are a few issues still to be addressed:

(1) The variable outfile is overwritten partway through mni_nl in surfinfo.py, which means that cortex.db.get_surfinfo(subject,type='mni_nl') does not save its output correctly. I fixed this by changing the intermediate outfile variable to tmpfile.

(2) In all other db.get_surfaceinfo calls, the variables that are saved are called left and right, which then are made into a cortex.dataset.views.Vertex object. The mni_nl() function gives the variables the names leftpts and rightpts. Also, the coordinates are transposed vs other types of surface info. These two things (variable names and coordinate orientation) prevented the npz file variables from being converted into a Vertex object.

Minor things:
Technically the second set of coordinates (currently labeled "magnet") are not magnet coordinates at all - they are indices into the original functional volume. Thus they should be integers, not floats. Based on what displays now, I think using ceil would be the best way to convert them to ints.

Also - I just spoke with James, who said you guys have been having side conversations about some of these exact things. Please to post here if there are changes to the code! I will abandon my updates if there are already fixes in progress.

@smerdis
Copy link
Author

smerdis commented Nov 18, 2015

Hey, good catch on the outfile being overwritten. I just fixed it in my code by changing the name of the supplied parameter to si_outfile. Is that an acceptable solution or do we want to enforce that first parameter being named outfile?

Also, which coordinates are transposed? This is probably related to the fact that our anatomicals are oriented differently than the standard, hence why I had to use fslreorient2std.

As a side note, do we really want the MNI coordinates to get transformed into Vertex objects?

@marklescroart
Copy link
Contributor

Mostly, we want consistency with other functions. So it makes more sense to me to change the internal variable name rather than the input variable name. And it does make more sense to me to have as consistent an output from the get_surfinfo function as possible. Some difference is necessary, because the MNI coordinates are 3 values per vertex instead of 1, but that seems to be automatically handled by converting them to a Vertex movie object (if the input is the right size). Also, to clarify, it isn't that x is transposed with z or anything, you just need to .T the left and right variables that are stored in the .npz file (not leftpts and rightpts) in order for them to be read into a Vertex object. This will also necessitate changing slightly the code in brainctm.py, but that should be fairly straightforward.

Also - I ran this on two subjects (MLfs2 and DSfs2), and got (in MLfs2, anyway) an error due to nans in the mni coordinates. It only appeared to be one vertex that was nan'd out, so I just wrapped the coordinates in np.nan_to_num - did you run into anything like that in MLfs2, Arjun?

… the intermediate file instead. Also fixed the output to match what other surfinfo functions do: now the hemispheres' data are called left and right (instead of leftpts and rightpts), and I transposed them (plus re-transpose in ctm load)
@smerdis
Copy link
Author

smerdis commented Nov 18, 2015

Okay, I re-patched the mni_nl() function to use tmpfile in the intermediate stages, and outfile at the end. Also fixed the naming and transpose issues, so the result should be a Vertex now.

I got a NaN or two with MLfs2, yeah. Probably a good idea to wrap in nan_to_num.

Still working on the mm-vox thing...

… other set (which i call 'magnet' in the code) is in voxel indices. both of these are correctly displayed and can be searched against and toggled between. also, the selected vertex's coords are displayed in the box after search.
@smerdis
Copy link
Author

smerdis commented Nov 19, 2015

Okay, I believe I've fixed the remaining issues. dfefa58 fixes the surfinfo filename and format, and c31c4e3 fixes the magnet-coords-in-vox-not-mm issue. the latter also fixes the bug where the coords of the search result vertex weren't pushed back to the coord box.

@marklescroart
Copy link
Contributor

Sorry to report that this still isn't quite ready to go. There were a few variable name changes that I had to fix in brainctm.py, and now that I have fixed them I'm still getting a CTM_INVALID_MESH error from openctm that I do not understand.

Arjun, I realize we're dragging this into your next rotation - can you possibly give me commit access to your repo? That way I can directly push the fixes that I've made there and just leave the pull request as it is.

@smerdis
Copy link
Author

smerdis commented Nov 30, 2015

Hey, sorry for the delay, you should have commit access to my repo.

@marklescroart
Copy link
Contributor

I do - I've just been bogged down with other stuff. I did try to push some
changes, but I fucked up the commit, and added it to another branch
(smerdis-master instead of master), and got annoyed trying to un-fuck it.

=-P

M

On Mon, Nov 30, 2015 at 9:25 AM, Arjun Mukerji [email protected]
wrote:

Hey, sorry for the delay, you should have commit access to my repo.


Reply to this email directly or view it on GitHub
#135 (comment).

Base automatically changed from master to main January 19, 2021 20:02
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.

3 participants