miykael / atlasreader

Python interface for generating coordinate tables and region labels from statistical MRI images

BSD 3-Clause "New" or "Revised" License

88 stars 32 forks source link

ENH: doc fixes and changed default files to have atlasreader as prefix #69

Closed danjgale closed 5 years ago

danjgale commented 5 years ago

Simple fixes for #68. Just changed the documentation for the filename parameter in create_output, and changed default output prefix to "atlasreader".

Now that I think about it, the use of filename is maybe a misnomer. Perhaps we could change it to img to show that it is not just a file name, but can also be any NIfTI-like object. Thoughts?

codecov-io commented 5 years ago

Codecov Report

Merging #69 into master will not change coverage. The diff coverage is 0%.

@@           Coverage Diff           @@
##           master      #69   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           6        6           
  Lines         348      348           
=======================================
  Hits          319      319           
  Misses         29       29

Impacted Files	Coverage Δ
atlasreader/atlasreader.py	`97.83% <0%> (ø)`	: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...9c2dffb. Read the comment docs.

miykael commented 5 years ago

Quick question, after your PR, does it accept filenames, as well as nifti images, like in nilearn? That would be ideal.

I think I would leave filename if it also accepts just strings, otherwise this might be confusing to certain users. And I think it's also coherent to nilearn's approach, no?

danjgale commented 5 years ago

It accepts filenames or nifti images, as in nilearn – I should've been a little clearer with that in the PR! Just edited it above.

nilearn typically names these parameters with _img (e.g., NiftiMasker).

miykael commented 5 years ago

I'm just worried that if we don't call the main input parameter not filename, users will feel obliged that they load the image first with nibabel.

But @rmarkello has more experience with nibabel and nilearn, let's see what he thinks.

rmarkello commented 5 years ago

Sorry for the delay in getting to this; a local conference in Montreal had me tied up last week! Just getting back to these things.

This all seems fine to me! Going to go ahead and merge.