search

miykael / atlasreader

Python interface for generating coordinate tables and region labels from statistical MRI images
BSD 3-Clause "New" or "Revised" License
89 stars 31 forks source link

Atlas and template revision #67

Closed miykael closed 6 years ago

miykael commented 6 years ago

This PR replaces https://github.com/miykael/atlasreader/pull/60 and therefore closes https://github.com/miykael/atlasreader/pull/60.

This PR updates the templates and atlases and adds a README file that describes everything, explains the origin and points to the different references and licenses.

Summary

I tried to make the commit messages self-explanatory but here's a short summary:

Additional Notes and open questions

  1. This PR adds two/three new atlases to the repo. The two talairach atlases and the AICHA atlas. Are you ok with this or should we drop some of them? Also, to not overload this PR, I decided to leave the integration of those new atlases to another PR. Does anybody want to do this?
  2. I prefer to normalize my data to the ICBM_asym_09c atlas, and therefore propose that we replace the default MNI152 template with this one. What do you think? Also, should we an additional input parameter/flag to the command, so that the user can decide if the plotting uses the MNI152 or the ICBM template?
  3. In the README file I'm linking to the corresponding licenses and I mention that FreeSurfer need registration to accept the license. Do you think it therefore is enough to ask users to read the atlas README file?
  4. The Neuromorphometrics atlas is a tricky one. To have access to the the atlas and also to please the license you would need to subscribe to their homepage. BUT, the atlas is also shipped by default with SPM12 (this is the version that we are using). I tried to link to both licenses in the readme file. But I'm also hesitating to kick this atlas out to prevent grayzones.
codecov-io commented 6 years ago

Codecov Report

Merging #67 into master will increase coverage by 0.23%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   91.66%   91.89%   +0.23%     
==========================================
  Files           6        6              
  Lines         348      358      +10     
==========================================
+ Hits          319      329      +10     
  Misses         29       29
Impacted Files Coverage Δ
atlasreader/atlasreader.py 97.89% <100%> (+0.05%) :arrow_up:
atlasreader/tests/test_atlasreader.py 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4b6a2e4...d28dce5. Read the comment docs.

rmarkello commented 6 years ago

Now THAT is a PR message :tada: :raised_hands:

I'm a bit busy this morning (EST), but will try and do a thorough read-through of the very detailed README this afternoon. From a first pass this looks awesome!

danjgale commented 6 years ago

Great PR. Like @rmarkello I'll look at the README as well!

danjgale commented 6 years ago

README looks good to me right now. I think it should work well in the interim while the long term solutions discussed in #60 are sorted out.

miykael commented 6 years ago

New Commit - Adding new atlases to code base

I've added a new commit (https://github.com/miykael/atlasreader/pull/67/commits/af8a6e8a8b64e1e3541b4dd487c40dc0a87944b6) that adds the new atlases to the codebase. Didn't think that it was so simple :-)

miykael commented 6 years ago

New commit - correcting harvard_oxford atlas and label file

The new version of the harvard_oxford atlas also contained the labels for Cerebral White Matter and Cereberal Cortex. This means that the probability for them most of the time was higher than for any other region. Therefore, I removed those four labels (left and right) fom the atlas and label file and described the "how" in the readme file with commit https://github.com/miykael/atlasreader/pull/67/commits/cb855b00ca1b5943398dd99dd084f71f9f9525ac.

miykael commented 6 years ago

New commit - atlas bounding box problem

This bug revealed a bug in the code. If the stat_map has a voxel that it wants to look up that is outside the bounding box of an atlas, it will crash, as it cannot find a label at said location. We therefore need a function that checks if the voxelID is within the atlas.

I decided to create a new function check_atlas_bounding_box that sets the voxelID to the origin (i.e. [0, 0, 0]) if a voxel is outside of the atlas. Like this we don't need to worry that the stat_map is always smaller than the atlas. Commit https://github.com/miykael/atlasreader/pull/67/commits/77ae56abb3dba3e54e50ba1a3f8b430613e94f4e adds this new function, plus a corresponding test.

miykael commented 6 years ago

New commit - atlas file size reduction and improving loading time

I wrote a script that crops the atlases to their minimal size (removes slices with just zeros) and compresses them with a specific level. To figure out the optimal setup, I benchmarked the hell out of it. Following are atlas loading time for 'all' or just 'juelich' (a rather big one), as well as the overall size of all atlases:

              all      juelich   size
orig          7.54s    3.45s     12.4 MB
crop_level1   4.21s    1.94s     12.5 MB
crop_level6   4.09s    1.87s      8.0 MB
crop_level9   4.11     1.87       7.8 MB

As you can see, using the cropped images leads to a 45% speed up. Note, I tested a compression level of 6, as it is the default level when using gzip.

Because of this, I've decided to upload the atlases as cropped version with a compression level of 6. This is done with the commit https://github.com/miykael/atlasreader/pull/67/commits/07e56715b7667011f3064e3e730c468a60e54152. This commit also contains the script to process/prepare the atlases.

rmarkello commented 6 years ago

Just went through the README and it looks great! I made one small comment on the doc-string of the check_atlas_bounding_box() function, but other than that I would be super happy to merge this in. All the work you did on this is awesome, and the documentation of everything is 🙌 💯 👍 🎉 .

miykael commented 6 years ago

@rmarkello thank you for the nice feedback. And you're right, the parameter description was from another doc-string that I've copied. It was my first nicely formated, numpy style, docstring :-) I've corrected it now with https://github.com/miykael/atlasreader/pull/67/commits/d28dce58c6fee7d086b985bd97d20e106bb84491.

If you're still happy you can go ahead and merge this.