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] Mean-center design matrix within getcoeffs #365

Closed
wants to merge 13 commits into from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jul 17, 2019

Closes #179. This is a minor refactor with a couple of diverse changes. It should not impact behavior except for minuscule changes to PCA metrics. I believe that the changes actually reflect a bug fix, but they're so small that I don't think it's worth it to refer to this as a "fix" PR.

Changes proposed in this pull request:

  • Mean-center IV matrix within getcoeffs.
  • Remove add_const argument from getcoeffs. Add constant automatically instead. This actually is not necessary, because mean-centering the IV matrix results in the same parameter estimates for some reason.
  • Remove unnecessary mean-centering of DV matrices prior to getcoeffs calls.
  • Use a pandas DataFrame for filling in comptable within dependence_metrics from the beginning. We used to create 1D numpy arrays, fill those in, stack them, and then make the DataFrame from the resulting 2D array. This simplifies things.
  • Drop an unreachable I006 classification from the TEDICA decision tree. This was a mistake on my part in [FIX] Add early escape from TEDICA decision tree #298. The mistake doesn't impact behavior at all, but makes the code less readable.
  • Allow getelbow to handle empty input arrays. If an input array is empty, return NaN with a warning. For getelbow calls where results are averaged or min-ed, use nanmean or nanmin instead.

@codecov
Copy link

codecov bot commented Jul 17, 2019

Codecov Report

Merging #365 into master will decrease coverage by 32.15%.
The diff coverage is 36.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #365       +/-   ##
===========================================
- Coverage   79.48%   47.33%   -32.16%     
===========================================
  Files          40       39        -1     
  Lines        2208     2212        +4     
===========================================
- Hits         1755     1047      -708     
- Misses        453     1165      +712
Impacted Files Coverage Δ
tedana/metrics/kundu_fit.py 29.71% <0%> (-65.82%) ⬇️
tedana/selection/_utils.py 23.25% <0%> (-69.25%) ⬇️
tedana/selection/tedica.py 24.06% <0%> (-64.18%) ⬇️
tedana/io.py 48.42% <0%> (-47.37%) ⬇️
tedana/stats.py 75.86% <100%> (-20.75%) ⬇️
tedana/tests/test_stats_get_coeffs.py 100% <100%> (ø) ⬆️
tedana/viz.py 9.86% <0%> (-82.9%) ⬇️
tedana/decomposition/_utils.py 27.77% <0%> (-66.67%) ⬇️
tedana/decomposition/pca.py 14.77% <0%> (-65.91%) ⬇️
... and 11 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 26fece1...c254025. Read the comment docs.

@stale
Copy link

stale bot commented Nov 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to tedana:tada: !

@stale stale bot added the stale label Nov 9, 2019
@tsalo
Copy link
Member Author

tsalo commented Nov 10, 2019

This PR sadly suffers from bloat, so I'm taking the core changes and putting them in a new PR. Closing this one now.

@tsalo tsalo closed this Nov 10, 2019
@tsalo tsalo deleted the light-refactor branch November 19, 2019 02:13
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.

get_coeffs run on mean-centered data for no reason
1 participant