*Bugfix and improvements to dimensionality estimation #150
Conversation
… multiple Wishart Distributions
| @@ -5,7 +5,17 @@ | |||
| %Variables | |||
| Out.VNDIM=VN; %Variance Normalization Dimensionality initially set to 1 | |||
| lnb = 0.5; %Lower noise bound for Golden Section Search | |||
| stabThresh = Iterate; %How many iterations meeting dimThresh criterion (1 for VNDIM=1 results, 2 for converged results) | |||
| stabThresh = Iterate; %How many iterations meeting dimThresh criterion (1 for VNDIM=1 results, 2 for legacy converged results, -1 for new converged results, >3 for fixed iterations) | |||
There was a problem hiding this comment.
It would be helpful if there was a brief explanation of all the inputs immediately following the function definition.
There was a problem hiding this comment.
I also didn't try to follow all the details. It would be helpful if you could add to the PR a brief summary of the implication of the cumulative changes, along the lines of what you explained to me verbally, to capture that within the PR itself.
I would suggest not merging this quite yet though, until we are ready to re-compile the matlab to go along with the changes.
|
This needs to be merged so that a collaborator can use it. FIX with compiled matlab is currently unsupported by FIX anyway. |
|
Well the FIX issue with compiled matlab should be resolved within a day or so, as Matthew and I now have a working version. We definitely don't want to tag a new HCPpipelines release until we have the matlab re-compiled that goes with this PR. (FWIW, your collaborator could have simply used your branch, thus avoiding the need to merge into the master before the matlab was re-compiled). |
|
If we cannot support compiled matlab, we shouldn't support it. For example, I don't think we should use that in Qu|Nex container. |
|
@glasserm what? You seem to be suggesting something pretty drastic. Outputs from compiled matlab should be the same as results from interpreted matlab, while octave might have some differences (someone mentioned differences in ill-conditioned matrix handling). |
|
Well if we cannot keep the compiled matlab in sync with interpreted matlab/Octave, we shouldn't support it. We cannot have the interpreted code "locked in" by problems with keeping the compiled matlab code up to date. |
|
As long someone with push access to the pipelines has the right version of matlab installed, they just have to run the compile script to update the compiled code (and new matlab files have to be added to the compile script when first created). Saying that we "can't" keep them in sync is wrong. |
|
If it is that easy, then it shouldn't hold up this pull request. |
|
It should be relatively easy going forward once @junilc does it once (thanks to the compile scripts that TimB left behind). Right now the hold-up is trying to get a R2017b license. My main point was that your collaborator could have easily used the branch (rather than the master) for the time-being. |
This pull request improves the accuracy and stability of dimensionality estimation for MR+FIX and related code. In particular, the pca_dim.m function now aligns with the implementation in melodic, dimensionality is now estimated using the full set of normalized eigenvalues, rather than just the left most when using multiple Wishart distributions (i.e. the left most eigenvalues >1 are padded with ones to match the original number of eigenvalues), and when using multiple Wishart distributions a single overall Wishart distribution is fit to the estimated null eigenvalues for the purposes of the pca_dim.m estimation. Also, two new modes of convergence are now available for icaDim.m including -1, which converges when the incremental change in dimensionality estimate is less that 0.25, and numbers > 3, which just run the specified number of iterations and compute the average estimated dimensionality from this number.