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 BIDS output version checker #456

Merged
merged 8 commits into from
Feb 5, 2020
Merged

Conversation

mgxd
Copy link
Contributor

@mgxd mgxd commented Feb 3, 2020

started as nipreps/fmriprep#1526, but should be extendible to any package producing BIDS compliant outputs

@pull-assistant
Copy link

pull-assistant bot commented Feb 3, 2020

Score: 0.61

Best reviewed: commit by commit


Optimal code review plan (3 warnings)

ENH: add BIDS output version checker

...flows/utils/tests/test_bids.py 67% changes removed in fix: apply @oesteban...

fix: play nice with py35

niworkflows/utils/bids.py 50% changes removed in Apply suggestions fr...

     Apply suggestions from code review

fix: apply @oesteban review, do not warn

niworkflows/utils/bids.py 50% changes removed in address code review

     Apply suggestions from code review

     doc: remove test cleanup

     tst: expected value from write_text()

     address code review

Powered by Pull Assistant. Last update 9b7d02b ... eec19be. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 3, 2020

Codecov Report

Merging #456 into master will decrease coverage by 3.52%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   61.12%   57.59%   -3.53%     
==========================================
  Files          61       61              
  Lines        6119     6129      +10     
  Branches      710      712       +2     
==========================================
- Hits         3740     3530     -210     
- Misses       2336     2545     +209     
- Partials       43       54      +11
Flag Coverage Δ
#masks ?
#reportlettests 100% <ø> (ø) ⬆️
#unittests 46.61% <90.9%> (+0.07%) ⬆️
Impacted Files Coverage Δ
niworkflows/utils/bids.py 88.88% <90.9%> (+0.13%) ⬆️
niworkflows/func/tests/test_util.py 0% <0%> (-100%) ⬇️
niworkflows/func/util.py 28% <0%> (-52%) ⬇️
niworkflows/interfaces/fixes.py 47.05% <0%> (-39.22%) ⬇️
niworkflows/engine/workflows.py 26.31% <0%> (-15.79%) ⬇️
niworkflows/interfaces/registration.py 52.15% <0%> (-9.92%) ⬇️
niworkflows/viz/utils.py 74.09% <0%> (-7.52%) ⬇️
niworkflows/interfaces/utils.py 40.71% <0%> (-7.39%) ⬇️
niworkflows/interfaces/ants.py 67.7% <0%> (-6.25%) ⬇️
niworkflows/interfaces/images.py 54.93% <0%> (-4.54%) ⬇️
... and 1 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 8e42c6c...eec19be. Read the comment docs.

niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
mgxd and others added 2 commits February 4, 2020 15:07
Co-Authored-By: Oscar Esteban <code@oscaresteban.es>
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
@mgxd
Copy link
Contributor Author

mgxd commented Feb 5, 2020

this is all set

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.

Final super nit-picky nitpicks.

niworkflows/utils/bids.py Outdated Show resolved Hide resolved
Examples
--------
>>> check_pipeline_version('1.1.1rc5', 'sample_dataset_description.json')
>>> check_pipeline_version('1.1.1rc5+129.gbe0e5158', 'sample_dataset_description.json')
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about dropping the private part of the version in this context, @effigies?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's probably fine? It should only bite us with people playing with :unstable, and they presumably know there are risks.

niworkflows/utils/bids.py Outdated Show resolved Hide resolved
niworkflows/utils/bids.py Show resolved Hide resolved
niworkflows/utils/bids.py Show resolved Hide resolved
niworkflows/utils/bids.py Outdated Show resolved Hide resolved
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.

Does it make sense to check both the pipeline name and version?

@mgxd
Copy link
Contributor Author

mgxd commented Feb 5, 2020

I based this on the assumption the dataset_description.json would already be in that pipeline's output directory, but could add a secondary check if desired

@effigies
Copy link
Member

effigies commented Feb 5, 2020

Yeah, this is fine. We can add features in the future.

@mgxd mgxd merged commit 4fedc3a into nipreps:master Feb 5, 2020
@mgxd mgxd deleted the enh/compare-versions branch February 5, 2020 20:44
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.

3 participants