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

[DOC] refactor and update notebooks #802

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Remi-Gau
Copy link
Contributor

@Remi-Gau Remi-Gau commented Dec 16, 2021

Suggested on the maintainers mattermost discussion.

New notebooks can be previewed here: https://github.com/Remi-Gau/pybids/tree/remi-update_notebooks/examples

  • split the notebooks into smaller ones that are more narrowly focused on a "topic" or set of functionalities
  • try to update the notebooks so they show case more of the pybids functionalities
    • using configuration
    • writing files

TODO before merging

  • remove nb_black calls from notebooks

@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #802 (99211f4) into master (9449fdc) will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   85.76%   85.90%   +0.13%     
==========================================
  Files          34       35       +1     
  Lines        3865     3882      +17     
  Branches      993      998       +5     
==========================================
+ Hits         3315     3335      +20     
- Misses        353      354       +1     
+ Partials      197      193       -4     
Impacted Files Coverage Δ
bids/layout/layout.py 88.28% <ø> (+0.37%) ⬆️
bids/__main__.py 0.00% <0.00%> (-33.34%) ⬇️
bids/layout/index.py 84.92% <0.00%> (-0.06%) ⬇️
bids/ext/__init__.py 100.00% <0.00%> (ø)
bids/modeling/statsmodels.py 85.33% <0.00%> (+0.50%) ⬆️
bids/layout/validation.py 82.35% <0.00%> (+3.78%) ⬆️

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 9449fdc...99211f4. Read the comment docs.

@Remi-Gau
Copy link
Contributor Author

While running the notebooks I am getting a warning on one of the test datasets about its dataset_description.

Is this intentional ? Maybe to test the CLI pybids update ?

root = os.path.join(get_test_data_path(), "synthetic")
layout2 = BIDSLayout(root, derivatives=True)
/home/remi/github/pybids/bids/layout/validation.py:151: UserWarning: The PipelineDescription field was superseded by GeneratedBy in BIDS 1.4.0. You can use ``pybids upgrade`` to update your derivative dataset.
  warnings.warn("The PipelineDescription field was superseded "

@effigies
Copy link
Collaborator

No, the test dataset predates the derivatives release.

@Remi-Gau
Copy link
Contributor Author

No, the test dataset predates the derivatives release.

Want a quick separate PR to fix this ?

@effigies
Copy link
Collaborator

Sure. Might be good to keep the old one to test upgrade, and then use the upgraded one for smooth notebooking.

@Remi-Gau
Copy link
Contributor Author

Quick question so I don't non-sense in those examples.

Is it the expected behavior that parse_file_entities cannot recover the subject entity when only given the filename instead of the relative path?

from bids import BIDSLayout
from bids.tests import get_test_data_path
import os

data_path = os.path.join(get_test_data_path(), "7t_trt")
print(f"Initializing layout with: {data_path}")

layout = BIDSLayout(data_path)

# relative path
layout.parse_file_entities("sub-01/ses-1/anat/sub-01_ses-1_T1map.nii.gz")
>>> 
{'subject': '01',
 'session': '1',
 'suffix': 'T1map',
 'datatype': 'anat',
 'extension': '.nii.gz'}

# filename only
layout.parse_file_entities("sub-01_ses-1_T1map.nii.gz")
>>> 
{'session': '1', 'suffix': 'T1map', 'extension': '.nii.gz'}

That the datatype is not recovered from just the filename makes sense, but I am not sure why the subject is not parsed? Something I am missing?

Similarly using get_file with just the filename returns None

@effigies
Copy link
Collaborator

Guessing that parse_file_entities expects a full path, so layout.parse_file_entities("sub-01/ses-1/anat/sub-01_ses-1_T1map.nii.gz"), but I would expect it to do its best without the full path.

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.

None yet

2 participants