Closed raeedcho closed 2 years ago
Thanks for catching this. I must admit I've never really used the FA implementation for any real purpose and it was obviously shoehorned it into the PCA implementation. Sounds like we should remove this functionality for now and re-implement using the scores output just to be sure it's all correct?
I would say re-implementing with the scores output would make sense, if I could figure out how to estimate scores on new data with an already fit model using factoran
. Thinking about it more, I believe that it would be pretty simple to use fastfa
and fastfa_estep
to do it. The code already uses orthogonalize
from the GPFA toolbox, so it wouldn't introduce any extra dependencies.
Not sure if this is how we want to do it here, but I submitted a pull request with a fix (https://github.com/NeuralAnalysis/TrialData/pull/12). @mattperich, take a look if you want, but if you don't care too much, I'll just merge it.
I'm not 100% sure, but I think the implementation of factor analysis in the
dimReduce
function is not quite correct. In particular, the projection of the data into factor space just post-multiplies the signal matrix byw
, but the result doesn't match thescores
output offactoran
. Also, my understanding is thatw
for factor analysis is the loading matrix to convert factors into original signals, not the other way around--to go backwards, there needs to be some sort of inversion of the matrix, or an explicit estimation of the posterior mean of the latents.Possible fix would be to use
fastfa
to estimate the model andfastfa_estep
to do the posterior estimation--both functions are from Byron Yu's GPFA codepack, which is included in thetd_limblab
folder.