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

ENH: Add optional session filter when collecting data #678

Merged
merged 3 commits into from
Mar 29, 2022

Conversation

mgxd
Copy link
Contributor

@mgxd mgxd commented Jan 24, 2022

A slight alteration to provide session level control.

niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
@mgxd
Copy link
Contributor Author

mgxd commented Jan 25, 2022

@effigies looks like using Query.ANY is filtering out the files...is this a pybids bug?

(Pdb) layout.get(
                return_type="file",
                subject=participant_label,
                session=session_id,
                extension=[".nii", ".nii.gz"], **queries['fmap'])
[]
(Pdb) session_id
<Query.ANY: 2>
(Pdb) layout.get(
                return_type="file",
                subject=participant_label,
                extension=[".nii", ".nii.gz"], **queries['fmap'])
['/Users/mathiasg/code/niworkflows/../nibabies/testdata/roo/sub-01/fmap/sub-01_dir-AP_epi.nii.gz', '/Users/mathiasg/code/niworkflows/../nibabies/testdata/roo/sub-01/fmap/sub-01_dir-PA_epi.nii.gz']

@mgxd
Copy link
Contributor Author

mgxd commented Jan 25, 2022

Actually, I think this is expected

  • Query.NONE: The named entity must not be defined.
  • Query.ANY: the named entity must be defined, but can have any
    value.

Perhaps there needs to be another value for Query that mimics ANY, but allows undefined entities as well.

@effigies
Copy link
Member

Oh damn.

     filters : dict
        Any optional key/values to filter the entities on.
        Keys are entity names, values are regexes to filter on. For
        example, passing filters={'subject': 'sub-[12]'} would return
        only files that match the first two subjects. In addition to
        ordinary data types, the following enums are defined (in the
        Query class):
            * Query.NONE: The named entity must not be defined.
            * Query.ANY: the named entity must be defined, but can have any
                value.

I guess we don't currently have a query enum that would do what I was thinking. I guess we need to revert to what you had, but let's open an issue on PyBIDS to allow a query that means it can be present or absent (maybe Query.OPTIONAL?).

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

As per the new docstring, session_id can be None. If so, it would be better to make the value replacement the latest possible, rather than the default parameter.

@@ -145,6 +146,7 @@ def collect_participants(
def collect_data(
bids_dir,
participant_label,
session_id=Query.ANY,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
session_id=Query.ANY,
session_id=None,

niworkflows/utils/bids.py Show resolved Hide resolved
@oesteban
Copy link
Member

let's open an issue on PyBIDS to allow a query that means it can be present or absent (maybe Query.OPTIONAL?).

Perhaps Query.EITHER (somewhat closer to enthought traits)?

@effigies
Copy link
Member

bids-standard/pybids#809

@oesteban
Copy link
Member

OMG you guys are fast :)

@mgxd mgxd marked this pull request as draft February 3, 2022 15:56
@mgxd mgxd added the on hold label Feb 7, 2022
mgxd and others added 2 commits March 29, 2022 13:28
@mgxd mgxd force-pushed the enh/collect-data-session branch from ff3899a to 61a1df9 Compare March 29, 2022 17:29
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #678 (22ddb99) into master (3d10143) will decrease coverage by 1.76%.
The diff coverage is 0.00%.

❗ Current head 22ddb99 differs from pull request most recent head 620ce08. Consider uploading reports for the commit 620ce08 to get more accurate results

@@            Coverage Diff             @@
##           master     #678      +/-   ##
==========================================
- Coverage   49.14%   47.38%   -1.77%     
==========================================
  Files          49       45       -4     
  Lines        5858     5605     -253     
  Branches      834      808      -26     
==========================================
- Hits         2879     2656     -223     
+ Misses       2871     2854      -17     
+ Partials      108       95      -13     
Flag Coverage Δ
reportlettests ∅ <ø> (∅)
unittests 47.38% <0.00%> (-1.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
niworkflows/utils/bids.py 72.81% <0.00%> (-2.19%) ⬇️
niworkflows/viz/plots.py 5.94% <0.00%> (-16.19%) ⬇️
niworkflows/interfaces/confounds.py 22.92% <0.00%> (ø)
niworkflows/testing.py
niworkflows/utils/timeseries.py
niworkflows/utils/testing.py
niworkflows/interfaces/morphology.py
niworkflows/interfaces/bids.py 96.77% <0.00%> (+0.55%) ⬆️
niworkflows/interfaces/freesurfer.py 49.21% <0.00%> (+0.78%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d10143...620ce08. Read the comment docs.

@mgxd mgxd removed the on hold label Mar 29, 2022
@mgxd mgxd marked this pull request as ready for review March 29, 2022 20:12
Copy link
Member

@effigies effigies 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.

@effigies effigies merged commit cfb9e3f into nipreps:master Mar 29, 2022
@mgxd mgxd deleted the enh/collect-data-session branch March 29, 2022 20:30
@mgxd mgxd restored the enh/collect-data-session branch July 1, 2022 12:41
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.

4 participants