extract defacing code to a function

[leej3]

Hi great tool. Thank you. I've been working with this a lot recently.

[vsoch]

hey @leej3 ! This is a good idea, I've done similar for a lot of tools so I can use it from within python as well as from the command line. One quick suggestion might be to have the function represented on the level of the variables after they are extracted from args. You can imagine that a user that wants to use the function in his own script would have a hard time assembling an args object, whereas it is easy to define the components like input files and settings.

[leej3]

Great. thanks for the suggestion. that's a nicer way of doing it.

[leej3]

I added a debug option too. shamelessly stolen from the heudiconv codebase

[vsoch]

This should be good! I don't see tests on the repository, so I think we should set that up and use your case as the first go. For example, for the two cases when you use os.remove, if there is some condition where the files are removed (either by another function, a user, or an error running the task) the os.remove will result in error. Here is a quick example:

import os
os.remove('idontexst.txt')
---------------------------------------------------------------------------
FileNotFoundError                         Traceback (most recent call last)
<ipython-input-3-d379509cc59b> in <module>()
----> 1 os.remove('idontexst.txt')

FileNotFoundError: [Errno 2] No such file or directory: 'idontexst.txt'

Would you be up for writing a simple test for your function? I can look into hooking into a CI to test it out.

[leej3]

Yep that sounds good. I'll add a few tests. I can push a dockerfile If we want to use a docker container on circleci the container I have been using was created with:

docker pull kaczmarj/neurodocker:master docker run --rm kaczmarj/neurodocker:master generate \ --base debian:stretch --pkg-manager apt \ --fsl version=5.0.10 \ --user root \ --install git vim \ --user neuro \ --miniconda env_name=neuro \ conda_opts="--channel conda-forge" \ conda_install="python nipype nibabel" \ pip_install="git+git://github.com/leej3/pydeface.git" \ activate=true \ --workdir /home/neuro \ --no-check-urls > Dockerfile

[chrisgorgo]

Thanks for pushing this forward! Tests indeed would be fantastic.

Best, Chris

On Tue, Feb 27, 2018 at 1:28 PM, john lee notifications@github.com wrote:

Yep that sounds good. I'll add a few tests. If we want to use a docker container on circleci the container I have been using was created with:

docker pull kaczmarj/neurodocker:master docker run --rm kaczmarj/neurodocker:master generate --base debian:stretch --pkg-manager apt --fsl version=5.0.10 --user root --install git vim --user neuro --miniconda env_name=neuro conda_opts="--channel conda-forge" conda_install="python nipype nibabel" pip_install="git+git://github.com/leej3/pydeface.git" activate=true --workdir /home/neuro --no-check-urls > Dockerfile

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/poldracklab/pydeface/pull/19#issuecomment-369032247, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOkp9_w3scbajYKuqbFfD4Ji43nvkoFks5tZHNygaJpZM4STYHS .

[leej3]

here are some tests and the dockerfile. The dockerfile doesn't always build for me. I have the image at timdanaos/pydeface. Not ideal for tests though. And it's huge. Maybe neurodocker's minify could help with that. I haven't properly checked it out yet.

I am still troubleshooting an error in the code. I seem to be alternating between an error in which the output file does not exist and nipype says it should and an error where I create it before hand with tempfile and then the output from the defacing can't be read by nibabel. shall let you know when i figure it out.

[leej3]

ok happy with this for now. let me know if you want any additions

[chrisgorgo]

This looks great! Especially the tests!

Would it be too much to ask for one last remaining step - circle or travis config file? Let me know what do you think.

[leej3]

Yep. Sounds good. I'm away at the moment but will sort that out next week

[Shotgunosine]

Alright, Circle is set up and passing. @vsoch do you want to take another pass here or should I go ahead and merge given that most of the other changes were reviewed about a year ago.

[vsoch]

I'm not lead maintainer here so it's not my say about merging, but I have one more quick question. I notice that you are setting a working directory and adding an addition user (neuro) to the docker container. Given the use case of this running on HPC (and using Singularity) will the container work as expected?

[Shotgunosine]

I just experimented with building the docker container, converting it to a singularity container and then copying that to our HPC and running it and it seemed to work fine. Any issues that come up with that process can be addressed in a future PR.

[vsoch]

Excellent. Great work @Shotgunosine!

[Shotgunosine]

Thanks!

On Mon, Feb 4, 2019 at 6:17 PM Vanessa Sochat notifications@github.com wrote:

Excellent. Great work @Shotgunosine https://github.com/Shotgunosine!

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/poldracklab/pydeface/pull/19#issuecomment-460452521, or mute the thread https://github.com/notifications/unsubscribe-auth/AFC1mLab4jG_S2t7chhJTlwE3RjWHTGGks5vKL-SgaJpZM4STYHS .