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 CIFTI-2 support #239

Merged
merged 35 commits into from Jul 15, 2020
Merged

Conversation

effigies
Copy link
Collaborator

@effigies effigies commented Jun 21, 2020

This builds on @satra's PR (#126).

It also supersedes #210, although that could be done separately. We should probably release PyBIDS 0.11 once this is working with master.

Still some things to be done, but this gets through run-level models on my test data.

TODO:

  • Set up tests with ds003
  • Test second-level
  • Fix/skip plotting for CIFTI-2
  • Figure out model maps with CIFTI-2
  • Probably some other bits I haven't thought of yet.

@pep8speaks
Copy link

pep8speaks commented Jun 21, 2020

Hello @effigies, Thank you for updating!

Line 177:100: E501 line too long (100 > 99 characters)

Line 3:1: F401 'os' imported but unused

To test for issues locally, pip install flake8 and then run flake8 fitlins.

Comment last updated at 2020-07-14 23:29:17 UTC

@effigies
Copy link
Collaborator Author

effigies commented Jul 9, 2020

@satra I'm currently able to get this to fit at the run level and do a fixed-effects combination at the subject level. The reporting needs work (subject level shows run 1) and the group level is crashing due to a failure to filter inputs properly, but feel free to give this a try and see if you're getting sensible results at the first/second level.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2020

Codecov Report

Merging #239 into master will decrease coverage by 5.18%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
- Coverage   78.13%   72.95%   -5.19%     
==========================================
  Files          19       19              
  Lines        1084     1198     +114     
  Branches      191      214      +23     
==========================================
+ Hits          847      874      +27     
- Misses        148      227      +79     
- Partials       89       97       +8     
Flag Coverage Δ
#ds003 72.95% <44.44%> (-5.19%) ⬇️
Impacted Files Coverage Δ
fitlins/workflows/base.py 64.13% <ø> (ø)
fitlins/interfaces/visualizations.py 61.71% <10.20%> (-32.11%) ⬇️
fitlins/cli/run.py 83.84% <33.33%> (-0.77%) ⬇️
fitlins/interfaces/nistats.py 70.23% <41.79%> (-14.66%) ⬇️
fitlins/__init__.py 76.47% <75.00%> (-1.31%) ⬇️
fitlins/interfaces/bids.py 73.68% <85.71%> (+0.52%) ⬆️
fitlins/interfaces/abstract.py 100.00% <100.00%> (ø)
fitlins/viz/reports.py 90.00% <100.00%> (+0.44%) ⬆️

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 ab47bf2...0b6188c. Read the comment docs.

@effigies effigies marked this pull request as ready for review July 14, 2020 23:58
@effigies
Copy link
Collaborator Author

The current version of ds003 that I'm testing on doesn't have usable CIFTI-2 files. I'm planning on re-running, but I'm not sure it's worth holding this up for that.

@adelavega Can you test neuroscout on this branch, or do I need to merge into master first?

@effigies effigies merged commit 5fe025b into poldracklab:master Jul 15, 2020
@effigies effigies deleted the enh/ciftilyze branch July 15, 2020 14:54
@bpinsard
Copy link

bpinsard commented Aug 5, 2020

Thanks for that nice feature!
Does smoothing works on the CIFTI space, prior to computing the maps?

@effigies
Copy link
Collaborator Author

effigies commented Aug 5, 2020

No, I don't have smoothing implemented for CIFTI, yet.

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

5 participants