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

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

Closed
tsalo opened this issue Jan 9, 2019 · 5 comments
Closed

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

tsalo opened this issue Jan 9, 2019 · 5 comments
Labels
question issues detailing questions about the project or its direction TE-dependence issues related to TE dependence metrics and component selection

Comments

@tsalo
Copy link
Member

tsalo commented Jan 9, 2019

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:

  • writeresults: data is not demeaned. add_const is not provided, so it defaults to False.
  • split_ts: X is ICA mixing matrix and data is demeaned data. add_const is not provided, so it defaults to False.
  • write_split_ts: X is ICA mixing matrix and data is demeaned data. add_const is not provided, so it defaults to False.
  • fitmodels_direct: X is ICA or PCA mixing matrix and data is demeaned optimally combined data. add_const is not provided, so it defaults to False.
  • fitmodels_direct (again): X is ICA or PCA mixing matrix and data is concatenated data (catd; not demeaned, afaik). add_const is not provided, so it defaults to False.
  • computefeats2: X is ICA or PCA mixing matrix and data is demeaned. add_const is not provided, so it defaults to False.

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 tsalo added the question issues detailing questions about the project or its direction label Jan 9, 2019
@tsalo
Copy link
Member Author

tsalo commented Feb 19, 2019

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
Copy link
Collaborator

jbteves commented May 23, 2019

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?

@tsalo tsalo added the TE-dependence issues related to TE dependence metrics and component selection label Oct 4, 2019
@CesarCaballeroGaudes
Copy link
Contributor

CesarCaballeroGaudes commented Nov 15, 2019

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:

  • I have changed the function to compute z-values, based on conversion from t-statistics, based on the following code The computations basically follow those indicated in any linear regression textbook, i.e. t-value = beta / std(beta), for instance see section Estimation in Ordinary Least Squares Wikipedia

  • Regardless the issue of mean centering design matrix, my opinion is that get_coeffs should only do OLS estimation regardless of the design (i.e. mixing) matrix and enable the option of adding a constant regressor (i.e. intercept). If the design matrix must have demeaned regressors, it should be done outside get_coeffs. Similarly, any data normalization must be done outside the function.

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 #458 (comment)

@emdupre
Copy link
Member

emdupre commented Nov 24, 2019

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
Copy link

stale bot commented Feb 22, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question issues detailing questions about the project or its direction TE-dependence issues related to TE dependence metrics and component selection
Projects
None yet
4 participants