yeatmanlab / pyAFQ

Automated Fiber Quantification ... in Python
http://yeatmanlab.github.io/pyAFQ/
BSD 2-Clause "Simplified" License
56 stars 34 forks source link

Adds engine:serial default #1141

Closed teresamg closed 4 months ago

pep8speaks commented 4 months ago

Hello @teresamg! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 165:9: E303 too many blank lines (2)

arokem commented 4 months ago

Thanks for adding this! IIUC, this is a followup to #1124?

Could you say a little about the errors that you are seeing without the addition of this line?

teresamg commented 4 months ago

GroupAFQ's parallel_params defaults to engine: serial, and this should still happen when using ParallelGroupAFQ. If there were errors with this line we can try something else?

arokem commented 4 months ago

Yeah, I think for now users would have to add: "engine" : "serial" to their code, along the lines of:

import os.path as op

from AFQ.api.group import ParallelGroupAFQ
from AFQ.definitions.image import RoiImage
import AFQ.api.bundle_dict as abd
import AFQ.data.fetch as afd

_, bids_path = afd.fetch_hbn_preproc(
        ["NDARZT957CWG",
         "NDARZU279XR3",
         "NDARZU401RCU",
         "NDARZU822WN3",
         "NDARZV421TCZ",
         "NDARZW262ZLV",
         "NDARZX163EWC",
         ],
        path="/gscratch/scrubbed/arokem/data/")

my_afq = ParallelGroupAFQ(
    bids_path=bids_path,
    preproc_pipeline="qsiprep",
    parallel_params={
        "engine" : "serial",
        "submitter_params": {
            "plugin": "slurm",
            "sbatch_args": "-J test \
                            -p ckpt \
                            --nodes=1 \
                            --cpus-per-task=8 \
                            --gpus=1 \
                            -A escience \
                            --mem=64G \
                            --time=2:00:00 \
                            -o /gscratch/scrubbed/arokem/logs/test.out \
                            -e /gscratch/scrubbed/arokem/logs/test.err \
                            --mail-user=arokem@uw.edu \
                            --mail-type=ALL"
        },
        "cache_dir": "/gscratch/scrubbed/arokem/tmp"
    }
)

my_afq.export_all()

Is that correct?

teresamg commented 4 months ago

It should run, but users won't always know to specify engine: serial, and don't have to unless specifying other parameters, so I think the default should be in there somehow!

36000 commented 4 months ago

This makes sense to me! Merging