ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
162 stars 95 forks source link

get_coeffs run on mean-centered data for no reason #179

Closed tsalo closed 4 years ago

tsalo commented 5 years ago

The function get_coeffs performs a least-squares fit of parameter X (generally a mixing matrix) on parameter data (generally optcom or other data). It is used in the following locations:

As far as I can tell, add_const isn't used anywhere in the package. That said, I also realized that demeaning data along axis 1 (which is how it is done when it is done) does not affect the results, so I'm not sure why we do it.

A really weird behavior that I don't understand at all is, when Y is not mean-centered, X gives the correct beta (with no need for an intercept) if X is mean-centered, but not if it's not. Does anyone know why that is?

Still, regardless of why that's the case, I think that we just need to mean-center X in get_coeffs, and we can stop mean-centering data and can also drop the add_const argument (should be unnecessary as long as X is mean-centered and it's never used anyway). This should not affect the results, but will make the code much easier to understand. How does that sound?

tsalo commented 5 years ago

Just to follow this up, the demeaning of data only has no impact when X (generally mmix) is also demeaned. I noticed that there is a difference for the PCA metric calculation because the PCA mixing matrix is not mean-centered. We should do that inside get_coeffs, because I can't imagine a situation where we would want to run this on data without a constant or mean-centered IVs. The parameter weights wouldn't be useful in that case.

jbteves commented 5 years ago

I don't see any reason why the above couldn't be done. Can you point me to the blob where'd like someone to see why de-meaning messes up the beta values?

CesarCaballeroGaudes commented 4 years ago

I have been working on the get_coeffs and computefeats2 functions (i.e. stats.py), which were basically performing a ordinary least squares (OLS) estimation and computation of Z-values after Fisher transformation assuming that OLS estimates were correlation coefficients, which is not exactly the same. Two related points:

In such manner, get_coeffs would be a simple OLS estimation and computation of Z-values.

And these changes will also affect to the following pull request https://github.com/ME-ICA/tedana/pull/458#issuecomment-553587769

emdupre commented 4 years ago

Thanks @CesarCaballeroGaudes -- does this mean that you'll be adding these changes into that PR ? It'd be great if we could keep the PRs focused, if at all possible. So, please keep us up to date with what you're intending to change ! I think that can be separate from the improvements to the documentation (which are also very much needed !).

stale[bot] commented 4 years ago

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: !