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

[FIX] Drop gifti support #114

Merged
merged 1 commit into from
Aug 20, 2018
Merged

[FIX] Drop gifti support #114

merged 1 commit into from
Aug 20, 2018

Conversation

emdupre
Copy link
Member

@emdupre emdupre commented Aug 20, 2018

Closes #34.

Changes proposed in this pull request:

  • Drops gifti support. Images must instead be tedana-denoised before projecting to the surface.

@codecov
Copy link

codecov bot commented Aug 20, 2018

Codecov Report

Merging #114 into master will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   49.25%   49.58%   +0.33%     
==========================================
  Files          32       32              
  Lines        2067     2033      -34     
==========================================
- Hits         1018     1008      -10     
+ Misses       1049     1025      -24
Impacted Files Coverage Δ
tedana/model/fit.py 28.38% <ø> (+0.23%) ⬆️
tedana/tests/test_utils.py 100% <100%> (ø) ⬆️
tedana/utils/utils.py 95.71% <100%> (+12.38%) ⬆️

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 f00dc40...9d39a27. Read the comment docs.

@emdupre
Copy link
Member Author

emdupre commented Aug 20, 2018

@tsalo @rmarkello please let me know if you foresee any issues with merging this !

@tsalo
Copy link
Member

tsalo commented Aug 20, 2018

Nothing jumps out at me in the code, and since tests are passing I think it should be good.

@emdupre
Copy link
Member Author

emdupre commented Aug 20, 2018

Dropping gifti support is a little bit of an extreme response to the discussion in #34, but I think we agreed on gitter that this was the best course of action in the long run. Just trying to make sure I'm not misrepresenting / misremembering that discussion !!

@rmarkello
Copy link
Member

As @tsalo said, nothing in the code strikes me as particularly concerning! And I think this is probably the right way forward, so 👍 from me.

@emdupre
Copy link
Member Author

emdupre commented Aug 20, 2018

Great, thanks for the feedback ! Merging !

@emdupre emdupre merged commit 645f277 into ME-ICA:master Aug 20, 2018
@emdupre emdupre deleted the remove-gii branch August 20, 2018 19:22
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