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

[REF] Replace MAPCA code with mapca library #641

Merged
merged 4 commits into from
Mar 17, 2021
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jan 3, 2021

Closes #698.

Changes proposed in this pull request:

  • Remove MA-PCA-related code from tedana.
  • Use new mapca library (once it's released on PyPi).

Base automatically changed from master to main February 1, 2021 23:57
@tsalo tsalo marked this pull request as ready for review March 15, 2021 20:12
@tsalo tsalo changed the title [REF] Use mapca library [REF] Replace MAPCA code with mapca library Mar 15, 2021
@tsalo tsalo requested review from eurunuela and jbteves March 15, 2021 20:19
jbteves
jbteves previously approved these changes Mar 15, 2021
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

This is GREAT! Thanks so much for everyone who moved it into its own package, great job!

@notZaki
Copy link
Contributor

notZaki commented Mar 15, 2021

The output might be different because mapca performs normalization along temporal-domain, while the implementation in tedana is along spatial-domain. At least, I think that's the case.

The license might also be worth discussing: mapca is currently licensed under GPLv2, and I think that merging it will require tedana to also change to GPL, and then any downstream dependencies such as fmriprep, would similarly have to change to GPL in order to use the next release of tedana.

eurunuela
eurunuela previously approved these changes Mar 15, 2021
Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

Changes look good, thanks @tsalo ! Something is wrong with the 3 echo test though.

@emdupre
Copy link
Member

emdupre commented Mar 15, 2021

The license might also be worth discussing: mapca is currently licensed under GPLv2, and I think that merging it will require tedana to also change to GPL, and then any downstream dependencies such as fmriprep, would similarly have to change to GPL in order to use the next release of tedana.

Changing the license is really a non-starter for both fMRIPrep and tedana, since it requires getting all existing contributors to the packages to re-consent to have their code release under the new license.

@tsalo
Copy link
Member Author

tsalo commented Mar 15, 2021

The license might also be worth discussing: mapca is currently licensed under GPLv2, and I think that merging it will require tedana to also change to GPL, and then any downstream dependencies such as fmriprep, would similarly have to change to GPL in order to use the next release of tedana.

I don't believe licensing impacts dependencies (i.e., I can have any licensed code as a dependency, but when including the actual code in my library it does matter). If anything, it was just a problem that we had MAPCA code within tedana before.

@jbteves
Copy link
Collaborator

jbteves commented Mar 15, 2021

The good news is that tedana is already GPLv2.1, so we don't have to change anything.
Per my understanding of the licensing: fMRIPrep can include tedana provided that there is a notice that tedana code is licensed under GPL. Since we're packaged on PyPI with a license notice both in the repository and on PyPI, they should be fine. They would run into trouble only if they started distributing our code with a different license (which they don't). If you're curious, they use the BSD license (allows derivative works without assent).

@notZaki
Copy link
Contributor

notZaki commented Mar 15, 2021

I don't believe licensing impacts dependencies (i.e., I can have any licensed code as a dependency, but when including the actual code in my library it does matter). If anything, it was just a problem that we had MAPCA code within tedana before.

True, the situation is kinda sticky with the current ma_pca code already in tedana.

The good news is that tedana is already GPLv2.1, so we don't have to change anything.

My understanding was that tedana uses L-GPL, which does not have the same restrictions as GPL [1] [2].

Based off this table/matrix it's ok to use a GPL library (mapca) inside L-GPL code (tedana) but the combination must be relicensed under the GPL (without the L).

@tsalo tsalo dismissed stale reviews from eurunuela and jbteves via eb0047f March 17, 2021 16:07
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #641 (eb0047f) into main (eec1a85) will decrease coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
- Coverage   93.70%   93.09%   -0.61%     
==========================================
  Files          26       25       -1     
  Lines        2032     1782     -250     
==========================================
- Hits         1904     1659     -245     
+ Misses        128      123       -5     
Impacted Files Coverage Δ
tedana/decomposition/__init__.py 100.00% <100.00%> (ø)
tedana/decomposition/pca.py 86.36% <100.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 eec1a85...eb0047f. Read the comment docs.

@tsalo tsalo requested a review from eurunuela March 17, 2021 17:05
Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @tsalo

@tsalo tsalo merged commit d67d9bd into ME-ICA:main Mar 17, 2021
notZaki pushed a commit to notZaki/tedana that referenced this pull request Mar 21, 2021
* Move mapca out of tedana.

* Pin to roughly 0.0.1.
@tsalo tsalo deleted the mapca-package branch March 22, 2021 23:05
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.

Replace internal MAPCA code with mapca library
5 participants