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

Add duecredit citation #6

Merged
merged 3 commits into from
May 15, 2017
Merged

Add duecredit citation #6

merged 3 commits into from
May 15, 2017

Conversation

effigies
Copy link
Member

@effigies effigies commented Mar 7, 2017

@yarikoptic: How does this look? And does it make more sense to put it at the module level for something this small, or to decorate the core function?

I haven't dug much into duecredit, so I'm not really sure why you'd choose one or the other.

Closes #5.

@yarikoptic
Copy link
Member

  1. I would recommend to decorate with due the quickshear function so no spurious citations if someone just merely imported your module (e.g. as a part of nipype interfaces or smth like that)

  2. but now that you would have two files to distribute (:-/) you would need to make a proper quickshear module with them, which would be advantageous to encourage you to get some quickshear/tests going ;) Otherwise, you can't just list due.py in your setup.py since you don't want it to be distributed as a top level module/package.

Alternative dirty solution-- copy/paste due.py into your quickshear.py so you stay as 1-file-module ;)

@yarikoptic
Copy link
Member

looks ok to me ;) did you try on some ? did it work? ;)
in my case on a quick trial I got it crashed:

$> ./quickshear.py /home/yoh/datalad_aside2/labs/haxby/raiders/sub001/anatomy/highres004_deface{d.nii.gz,mask.nii.gz} defaced         
Traceback (most recent call last):
  File "./quickshear.py", line 226, in <module>
    sys.exit(main(*sys.argv))
  File "./quickshear.py", line 220, in main
    new_anat = quickshear(anat_img, mask_img, opts.buffer)
  File "./quickshear.py", line 176, in quickshear
    xdiffs, ydiffs = np.diff(low)
ValueError: need more than 0 values to unpack

@effigies
Copy link
Member Author

effigies commented Mar 8, 2017

It's worked for me on the subjects I've tried. Could you make the brainmask available? This is an issue with the convex hull, it looks like, so I won't need the anatomical to assess.

Also, have you tried with v1.0? I did switch to using nice numpy functions where possible, but the hand-written code might give more informative errors. (Or work and show the things I thought equivalent weren't.)

@yarikoptic
Copy link
Member

all (well -- defaced and functionals) data is there online at http://datasets.datalad.org/?dir=/labs/haxby/raiders/sub001/anatomy/ . didn't crash with v1.0:

$> ./quickshear.py /home/yoh/datalad_aside2/labs/haxby/raiders/sub001/anatomy/highres004_deface{d.nii.gz,mask.nii.gz} defaced
Anat image axes: ('P', 'S', 'R')
Mask image axes: ('P', 'S', 'R')
Mask shape!: (256, 256, 160)
Aligning anatomical image to +x -> P
Aligning mask to +x -> P
Aligning anatomical image to +y -> S
Aligning mask to +y -> S
Defaced file: defaced

@effigies
Copy link
Member Author

@yarikoptic I see the problem. The code originally assumed (and still does) that the axes were always ('R'/'L', 'A'/'P', 'S'/'I'), and didn't consider permutations. I'll have to fix that...

@effigies
Copy link
Member Author

Also, that defacemask is not a brainmask, which is what quickshear needs. You're probably running into an issue with the fact that it's assuming you have a roughly brain-shaped mask.

@effigies effigies merged commit 10b3699 into nipy:master May 15, 2017
@effigies effigies deleted the duecredit branch May 15, 2017 19:37
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.

2 participants