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] Update config to support microscopy, qMRI, PET, ASL #840

Merged
merged 36 commits into from
Apr 24, 2022

Conversation

Remi-Gau
Copy link
Contributor

@Remi-Gau Remi-Gau commented Apr 14, 2022

closes #807

should cover both new entities and default path patterns

most of this was done by reading from the bids schema and relying on elbow grease and patience to do the rest.

Cannot wait for #818 to be done so I never have to do this by hand ever again but in the mean time: here we go.

Has not been properly tested yet:

  • path patterns

@Remi-Gau Remi-Gau requested review from effigies, tsalo, mariehbourget, HenkMutsaerts and agahkarakuzu and removed request for effigies April 14, 2022 12:09
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #840 (4c5e504) into master (c81e7f9) will increase coverage by 0.17%.
The diff coverage is n/a.

❗ Current head 4c5e504 differs from pull request most recent head 669200a. Consider uploading reports for the commit 669200a to get more accurate results

@@            Coverage Diff             @@
##           master     #840      +/-   ##
==========================================
+ Coverage   85.96%   86.13%   +0.17%     
==========================================
  Files          35       35              
  Lines        3932     3932              
  Branches     1018     1018              
==========================================
+ Hits         3380     3387       +7     
+ Misses        359      353       -6     
+ Partials      193      192       -1     
Impacted Files Coverage Δ
bids/layout/models.py 92.35% <0.00%> (+0.31%) ⬆️
bids/layout/index.py 87.35% <0.00%> (+0.79%) ⬆️
bids/layout/validation.py 84.70% <0.00%> (+2.35%) ⬆️
bids/conftest.py 92.00% <0.00%> (+8.00%) ⬆️

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 c81e7f9...669200a. Read the comment docs.

Makefile Outdated Show resolved Hide resolved
Comment on lines 61 to 72
@pytest.mark.xfail(reason="missing derivatives description: https://github.com/bids-standard/bids-examples/issues/310")
@pytest.mark.parametrize(
"dataset, nb_files",
[
("qmri_mpm", 125),
],
)
def test_layout_on_examples_with_derivatives(dataset, nb_files):
ds = join(get_test_data_path(), "bids-examples", dataset)
layout = BIDSLayout(ds, derivatives=True)
files = layout.get()
assert len(files) == nb_files
Copy link
Contributor Author

@Remi-Gau Remi-Gau Apr 18, 2022

Choose a reason for hiding this comment

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

Setting aside this test as an expected failure.

@agahkarakuzu you were right "some" failures are due to invalid derivatives in the qMRI bids-examples.
I checked and simply adding a valid dataset descritpion will fix this one.

See: bids-standard/bids-examples#310

@effigies
Copy link
Collaborator

@Remi-Gau the error is related to dropping a new data directory into the Python source tree.

I would suggest instead of doing this, that we create a git submodule in the project root, look for it in conftest and make it available as a fixture. Is that all clear, or would you like me to give it a shot in a separate PR?

@Remi-Gau
Copy link
Contributor Author

@Remi-Gau the error is related to dropping a new data directory into the Python source tree.

oh that's the pythonic way to say this? Noted. :-p

I would suggest instead of doing this, that we create a git submodule in the project root, look for it in conftest and make it available as a fixture. Is that all clear, or would you like me to give it a shot in a separate PR?

mostly clear but let's chat about all this tomorrow during our bids maintainers meeting because I have other questions about this PR that could benefit from a video call.

@Remi-Gau
Copy link
Contributor Author

for the record here is an example of what I get for "micr_SPIM"

ds = join(get_test_data_path(), "bids-examples", "micr_SPIM")
layout = BIDSLayout(ds)
files = layout.get(return_type="file")
print(files)

['/home/remi/github/pybids/bids/tests/data/bids-examples/micr_SPIM/dataset_description.json', 
'/home/remi/github/pybids/bids/tests/data/bids-examples/micr_SPIM/participants.json', 
'/home/remi/github/pybids/bids/tests/data/bids-examples/micr_SPIM/participants.tsv', 
'/home/remi/github/pybids/bids/tests/data/bids-examples/micr_SPIM/README']

Copy link
Contributor

@mariehbourget mariehbourget left a comment

Choose a reason for hiding this comment

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

Thank you @Remi-Gau for looking into this!

I tested the indexation of microscopy datasets with the BIDSlayout and everything is working great for me.

I was wondering if it would be possible to add the samples.tsv file to the get_collections function at the "dataset" level At the moment, the "dataset" level seems to only index metadata from participants.tsv.

For example on micr_SPIM with:

subj_df = layout.get_collections(level='dataset', merge=True).to_df()
subj_df

I get the following from participant.tsv, but samples.tsv metadata is not present:
image

bids/layout/config/bids.json Outdated Show resolved Hide resolved
bids/layout/config/bids.json Outdated Show resolved Hide resolved
bids/layout/config/bids.json Show resolved Hide resolved
@Remi-Gau
Copy link
Contributor Author

I was wondering if it would be possible to add the samples.tsv file to the get_collections function at the "dataset" level At the moment, the "dataset" level seems to only index metadata from participants.tsv.

For example on micr_SPIM with:

subj_df = layout.get_collections(level='dataset', merge=True).to_df()
subj_df

I get the following from participant.tsv, but samples.tsv metadata is not present: image

I suggest moving this to a different issue and fix it in a different PR

@Remi-Gau Remi-Gau requested a review from effigies April 24, 2022 12:21
@Remi-Gau
Copy link
Contributor Author

This is good for a final review for me.

Tests are still failing locally but that's a me problem.

Copy link
Collaborator

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

Lgtm.

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.

support for microscopy
4 participants