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

Track Sources for outputs with BIDS URIs #966

Merged
merged 64 commits into from
Oct 20, 2023
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Oct 10, 2023

Closes #568.

Breaking changes

  • Don't write out the temporal mask if fd_thresh is <= 0 (i.e., censoring is disabled).
  • Don't write out design file if params is None (i.e., denoising is disabled).

Changes proposed in this pull request

  • Plug BIDS URIs into appropriate JSON files with the Sources field.
  • Also add in a number of metadata fields required or recommended by BEP 012 (BEP 012: Functional MRI derivatives bids-standard/bids-specification#519).
  • Add DatasetLinks field to dataset description file. The links are to the preprocessed data, the XCP-D derivatives, and (optionally) the custom confounds.
  • Create a series of small functions (listed below) to generate BIDS URIs for different file locations:
    • _make_uri: General path-resolving function.
    • _make_xcpd_uri: Resolves path for XCP-D derivative files.
    • _make_xcpd_uri_lol: Resolves path for XCP-D derivative files organized in lists of lists..
    • _make_preproc_uri: Resolves path for preprocessing pipeline derivative files.
    • _make_custom_uri: Resolves path for custom confounds files.
  • Create some functions that help with managing the Sources field in workflows.
    • _listify: From NiMARE. Convert things to lists.
    • _make_dictionary: Feed kwargs into a dictionary. I use this to create metadata dictionaries containing Sources fields to pass into DerivativesDataSinks. The issue is that MapNodes need to reference fields in the .inputs attribute of the underlying interface, but DerivativesDataSinks don't include Sources as a field in .inputs. However, they do include meta_dict, so I need to pass in a meta_dict containing the Sources field instead of passing in Sources directly.
    • _transpose_lol: Transposes a list of lists. Used in _make_xcpd_uri_lol.

@tsalo tsalo added the enhancement New feature or request label Oct 10, 2023
@tsalo tsalo changed the title Track sources for some outputs Track Sources for some outputs with BIDS URIs Oct 10, 2023
@tsalo tsalo added this to the post-manuscript milestone Oct 11, 2023
@tsalo
Copy link
Member Author

tsalo commented Oct 11, 2023

The concatenated atlas-wise sources aren't right. Instead of selecting the appropriate atlas's element in the run-wise lists of sources, it's just keeping the full run-wise lists.

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Files Coverage Δ
xcp_d/utils/bids.py 88.50% <100.00%> (+1.62%) ⬆️
xcp_d/workflows/base.py 97.12% <ø> (ø)
xcp_d/workflows/bold.py 94.36% <ø> (ø)
xcp_d/workflows/cifti.py 93.65% <ø> (ø)
xcp_d/workflows/connectivity.py 97.00% <100.00%> (ø)
xcp_d/utils/utils.py 98.81% <88.23%> (-1.19%) ⬇️
xcp_d/workflows/concatenation.py 88.46% <77.77%> (+1.22%) ⬆️
xcp_d/workflows/outputs.py 92.03% <90.41%> (+4.80%) ⬆️

📢 Thoughts on this report? Let us know!.

@tsalo
Copy link
Member Author

tsalo commented Oct 16, 2023

@mattcieslak would you mind reviewing this with an eye toward streamlining the code? I'm sure I'll be able to improve it over time, but if there's anything obvious that I'm missing it would be great to catch it before I merge the PR.

@tsalo
Copy link
Member Author

tsalo commented Oct 16, 2023

Whoops, looks like the add-coverage-to-sources step is overwriting the sources without retaining the old ones. I'll need to figure out what went wrong with my recent changes before merging.

EDIT: Fixed!

@tsalo tsalo changed the title Track Sources for some outputs with BIDS URIs Track Sources for outputs with BIDS URIs Oct 18, 2023
@tsalo tsalo added the breaking-change PRs that change results or interfaces. label Oct 18, 2023
@tsalo tsalo mentioned this pull request Oct 18, 2023
4 tasks
@tsalo tsalo merged commit c6ef7b4 into PennLINC:main Oct 20, 2023
19 checks passed
@tsalo tsalo deleted the track-sources branch October 20, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change PRs that change results or interfaces. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant