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

use single pybids_inputs instead of two #104

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

use single pybids_inputs instead of two #104

wants to merge 6 commits into from

Conversation

akhanf
Copy link
Member

@akhanf akhanf commented Mar 6, 2025

Removes pybids_inputs_dwi.

Having two was problematic since the pybids_inputs_dwi wasn't changeable from the CLI options.

Proposed changes

Find/replace:
pybids_inputs_dwi -> pybids_inputs
inputs_t1 -> inpts
inputs_dwi -> inputs
mask -> dwi_mask

So far only dry-run testing done, but that should be sufficient given the change.

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you are unsure about any of the choices, don't hesitate to ask!

  • Changes have been tested to ensure that fix is effective or that a feature works.
  • Changes pass the unit tests
  • Code has been run through the poe quality task
  • I have included necessary documentation or comments (as necessary)
  • Any dependent changes have been merged and published

Notes

All PRs will undergo the unit testing before being reviewed. You may be requested to explain or make additional changes before the PR is accepted.

akhanf added 3 commits March 6, 2025 15:13
Removes pybids_inputs_dwi.

Having two was problematic since the pybids_inputs_dwi wasn't changeable
from the CLI options.
@akhanf akhanf marked this pull request as ready for review March 6, 2025 20:45
@akhanf akhanf requested a review from Dhananjhay March 9, 2025 18:26
Copy link

@Dhananjhay Dhananjhay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I was also able to get the dry test run working but I had to change the aparcaseg input parameter in the mgz2nii rule to point to the right location. However, I can't seem to test the wet run because it says permission denied when I try to write at the freesurfer's location.

@akhanf
Copy link
Member Author

akhanf commented Mar 10, 2025

interesting -- what data are you running it on? It would be good to have a small (single-subject) wet-run dataset to dig into that, perhaps could post an open access one to GIN?

@Dhananjhay
Copy link

Hmm I was just using the dummy test data that is shipped along with scattr. I run the following command for the dry run:

scattr scattr/test/data/bids derivatives participant --cores all -np --fs-license /srv/software/freesurfer/7.4.1/license.txt --freesurfer_dir /srv/software/freesurfer/7.4.1/

where I had to change

aparcaseg=str(
      Path(bids(root=freesurfer_dir, **inputs.subj_wildcards)).parent
      / "mri"
      / "aparc+aseg.mgz"
),

to

aparcaseg='/srv/software/freesurfer/7.4.1/subjects/fsaverage/mri/aparc+aseg.mgz'

For the wet run I couldn't find any suitable data from openneuro so I just tried the above command without the np flag just for experiment purposes but I guess it tries to write files at --freesurfer_dir /srv/software/freesurfer/7.4.1/ and therefore runs into permission issues. I agree re: (single-subject) wet-run and maybe we can extrapolate this into a Continuous integration (CI) test setup on github for scattr since it's pretty lightweight. What do you think?

@akhanf
Copy link
Member Author

akhanf commented Mar 11, 2025

Ah I see, actually the --freesurfer_dir cli arg is meant for pointing to where the freesurfer subjects dir is, ie where freesurfer has been run on the subject(s), not the freesurfer software dependency (that's in the container).

It also write files to the freesurfer folder when it runs the additional freesurfer thalamic subnuclei segmentation (since it runs atop freesurfer recon-all).

But for the wet run you would need to have freesurfer already run, as well as the diffusion data preprocessed, and the T1w brain-masked.

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.

3 participants