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.

Sign up for GitHub

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

Jump to bottom

Refactor HCP/DCAN ingression and fix converted filenames #714

Merged
merged 92 commits into from
Mar 10, 2023
Merged

Refactor HCP/DCAN ingression and fix converted filenames #714

merged 92 commits into from
Mar 10, 2023

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Dec 9, 2022 •
edited by kahinimehta
Loading

Related to #709.

To do:

  • Test on real data.
  • Figure out how to support nifti transform files. I assume that any tools that use the transforms can handle either file type, and the issue is just in collecting the appropriate files.

Changes proposed in this pull request

  • Replace string concatenation with os.path.join calls.
  • Keep track of input and output files with a dictionary, and write that dictionary out to the output directory.
  • Improve variable names.
    • Just as an example, midR was used for the HCP right-hemisphere midthickness file, while ``rMid` was used for the fMRIPrep version.

Documentation that should be reviewed

None.

@tsalo tsalo added the bug Issues noting problems and PRs fixing those problems. label Dec 9, 2022
tsalo
tsalo commented Dec 9, 2022
View reviewed changes
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented Dec 9, 2022

@kahinimehta it looks like there might be useful mappings in this file-mapper example.

tsalo
tsalo commented Dec 11, 2022
View reviewed changes
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Dec 11, 2022
View reviewed changes
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Dec 12, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py
tw1tonative = wmmask

# get task and idx run 01
func_dirx = dcan_dir + "/" + sub_id + "/ses-" + ses_id[0] + "/files/MNINonLinear/Results/"
Copy link
Member Author

Choose a reason for hiding this comment

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

AFAICT, this means that the script looped over all sessions, but copied functional files from only the first session's DCAN folder into all of the sessions' fMRIPrep folders.

tsalo
tsalo commented Dec 13, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py Outdated Show resolved Hide resolved
This was referenced Dec 14, 2022
Merged
Merged
@tsalo tsalo requested a review from kahinimehta December 14, 2022 22:34
tsalo
tsalo commented Dec 19, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Dec 19, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py
Comment on lines +77 to +82
# NOTE: Why *was* this set to the *first* session only? (I fixed it)
# AFAICT, this would copy the first session's files from DCAN into *every*
# session of the output directory.
func_dir_orig = os.path.join(anat_dir_orig, "Results")
func_dir_fmriprep = os.path.join(session_dir_fmriprep, "func")
os.makedirs(func_dir_fmriprep, exist_ok=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this was an important bug.

tsalo
tsalo commented Dec 19, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Dec 19, 2022
View reviewed changes
xcp_d/utils/dcan2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Mar 9, 2023
View reviewed changes
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Mar 9, 2023
View reviewed changes
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Mar 9, 2023
View reviewed changes
xcp_d/utils/bids.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Mar 9, 2023
View reviewed changes
xcp_d/utils/bids.py Outdated Show resolved Hide resolved
tsalo
tsalo commented Mar 9, 2023
View reviewed changes
Copy link
Member Author

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

The changes look really good, but there are a few minor things that I think need to be changed. The main thing is that I think all of the transforms should be text files, and should all use the identity transform you added.

kahinimehta added 2 commits March 9, 2023 13:03
@kahinimehta
Copy link
Collaborator

@tsalo should be good now!

tsalo
tsalo commented Mar 9, 2023
View reviewed changes
Copy link
Member Author

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

In order to ensure the scans.tsv file is accurate, you need to use the same key for all of the transforms, but add the values to a list.

xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
xcp_d/utils/hcp2fmriprep.py Outdated Show resolved Hide resolved
@tsalo tsalo merged commit 25d42d4 into PennLINC:main Mar 10, 2023
@tsalo tsalo deleted the hcp-dcan-ingression-refactor branch March 10, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perronea perronea perronea left review comments

@kahinimehta kahinimehta Awaiting requested review from kahinimehta

Assignees

@tsalo tsalo

Labels
bug Issues noting problems and PRs fixing those problems.
Projects
None yet
Milestone
manuscript
Development

Successfully merging this pull request may close these issues.
3 participants
@tsalo @kahinimehta @perronea