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

Refactor Correctors and remove statsmodels requirement #679

Merged
merged 21 commits into from
May 31, 2022

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented May 5, 2022

Closes #667.

Changes proposed in this pull request:

  • Replace calls to statsmodels multiple comparisons correction functions with PyMARE functions.
  • Remove nimare.stats.fdr.
  • Refactor Correctors somewhat.
    • Generic correction methods are now actual instance methods, with proper docstrings and everything!
    • Estimator methods explicitly override generic ones.
  • Rename MKDAChi2.correct_fwe_bh to MKDAChi2.correct_fwe_indep (breaking change).
  • Replace "fdr_bh" option with "bh" in decoder multiple comparisons correction (breaking change).

To do:

  • Call the appropriate correction method within the Corrector based on the method parameter.
  • Version directives.

@tsalo tsalo added refactoring Requesting changes to the code which do not impact behavior correct Issues related to the correct module maintenance Issues/PRs related to maintenance/infrastructure. labels May 5, 2022
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #679 (af2e588) into main (d37c216) will decrease coverage by 0.11%.
The diff coverage is 87.50%.

❗ Current head af2e588 differs from pull request most recent head fe822ae. Consider uploading reports for the commit fe822ae to get more accurate results

@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
- Coverage   85.32%   85.21%   -0.12%     
==========================================
  Files          41       41              
  Lines        4503     4483      -20     
==========================================
- Hits         3842     3820      -22     
- Misses        661      663       +2     
Impacted Files Coverage Δ
nimare/stats.py 98.97% <ø> (+4.74%) ⬆️
nimare/decode/discrete.py 92.34% <38.46%> (-4.27%) ⬇️
nimare/correct.py 94.28% <100.00%> (+1.21%) ⬆️
nimare/meta/cbma/mkda.py 96.96% <100.00%> (-0.13%) ⬇️
nimare/meta/utils.py 51.20% <0.00%> (-1.60%) ⬇️
nimare/meta/kernel.py 80.00% <0.00%> (-0.13%) ⬇️
nimare/io.py 95.07% <0.00%> (-0.08%) ⬇️
nimare/meta/cbma/base.py 95.63% <0.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 d37c216...fe822ae. Read the comment docs.

@tsalo tsalo added the breaking-change PRs that change results or interfaces. label May 6, 2022
@tsalo tsalo changed the title Remove statsmodels as a dependency Refactor Correctors and remove statsmodels requirement May 6, 2022
@tsalo tsalo requested review from adelavega and jdkent May 6, 2022 19:26
Copy link
Member

@jdkent jdkent left a comment

Choose a reason for hiding this comment

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

Looks good!

Main comment is I'm curious if the method parameter documentation could be defined once and referenced in other places, Don't consider it a blocking review comment, but it's nice not to have to repeat yourself, maybe rst substitutions would do what you want? https://stackoverflow.com/questions/8320909/repeated-parameters-with-python-sphinx

docs/conf.py Outdated Show resolved Hide resolved
nimare/stats.py Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented May 11, 2022

Main comment is I'm curious if the method parameter documentation could be defined once and referenced in other places, Don't consider it a blocking review comment, but it's nice not to have to repeat yourself, maybe rst substitutions would do what you want? https://stackoverflow.com/questions/8320909/repeated-parameters-with-python-sphinx

I think that's what nilearn does, and I certainly like it. Would you mind if I tackled that in a separate PR? I might open an issue about it.

Copy link
Member

@adelavega adelavega left a comment

Choose a reason for hiding this comment

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

Looks good to me, although I wonder if these non-imaging basic functions should perhaps live in pyMARE?

@tsalo
Copy link
Member Author

tsalo commented May 12, 2022

Looks good to me, although I wonder if these non-imaging basic functions should perhaps live in pyMARE?

That seems reasonable to me, but PyMARE doesn't use these methods at the moment, so we would probably only want to add them to that package if we also create a Corrector class within PyMARE as well.

@tsalo
Copy link
Member Author

tsalo commented May 25, 2022

@adelavega I've moved the new functions to PyMARE. I'll make a release once the associated PR (neurostuff/PyMARE#87) is merged.

EDIT: I've released PyMARE 0.0.4rc1 and pinned it here.

@tsalo tsalo requested review from adelavega and jdkent May 25, 2022 18:30
Copy link
Member

@adelavega adelavega left a comment

Choose a reason for hiding this comment

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

LGTM!

@tsalo
Copy link
Member Author

tsalo commented May 31, 2022

I've opened an issue about the parameter definitions (#687), so I'm going to resolve the conflicts and merge this once CI passes.

@tsalo tsalo merged commit 66ded4b into neurostuff:main May 31, 2022
@tsalo tsalo deleted the drop-statsmodels branch May 31, 2022 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change PRs that change results or interfaces. correct Issues related to the correct module maintenance Issues/PRs related to maintenance/infrastructure. refactoring Requesting changes to the code which do not impact behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace statsmodels dependency with internal multiple comparisons correction implementations
3 participants