trendscenter / coinstac

Collaborative Informatics and Neuroimaging Suite Toolkit for Anonymous Computation
MIT License
47 stars 19 forks source link

Change computation names #804

Closed everner closed 3 years ago

everner commented 4 years ago

Problem

The names for the computations are not how we want them. The ridge regression computations should say they're ridge regression, and there is no need to say 'normal equation'. I think it would actually be confusing for people to see 'normal equation', when really it's just regression. There are a few other minor improvements.

Tasks

Please make the following changes:

Regression (Singleshot) - VBM => Ridge Regression (Singleshot) - VBM Regression (Normal Equation) - VBM => Regression - VBM Regression (Singleshot) - FreeSurfer Volumes => Ridge Regression (Singleshot) - FreeSurfer Volumes Regression (Normal Equation) - FreeSurfer Volumes => Regression - FreeSurfer Volumes Decentralized DFNC => Decentralized Dynamic Functional Connectivity (DDFNC) Pipeline fMRI Preprocess => fMRI Preprocessor

hvgazula commented 4 years ago

@everner You cannot have ridge regression and single-shot in the same computation. You can do ridge regression in normal form (using moore-penrose pseudo-inverse) but it is otherwise not recommended. Only multi-shot can do regularization. If you agree with my comments, probably you may want to rechristen the names for the computations. Thanks!

@rssk It's high time the names are changed. We are reaching out to more people with coinstac and it will be sloppy to see unintuitive names. If you have any questions, please let me know and we can work this out together.

everner commented 4 years ago

@hvgazula If it's impossible to have single-shot ridge regression, then why does the "Regression (single shot)" compspec have lambda as an input parameter? In fact, if you look at the code, the function that performs the regression does regularization and takes lambda as a parameter. Am I missing something?

hvgazula commented 4 years ago

@everner Yes, indeed missing the point. The line of code you point to is never used in the computation. And regarding the compspec.json, it's always evolving. We literally stopped working on SSR a while ago. So, I'd not give it too much importance. For that matter, I am now of the opinion why will anyone want to use single-shot regression at all.

everner commented 4 years ago

To be honest, if there is no reason for someone to use this computation, then why should we keep it available? It will only waste our time maintaining it. If we don't maintain it, it will only confuse users and cause bugs. Let's discuss in meeting today.

everner commented 4 years ago

Update per the meeting we just had:

Regression (Singleshot) - VBM => Get rid of this whenever is convenient for you Regression - VBM => Regression - VBM Regression (Singleshot) - FreeSurfer Volumes => Get rid of this whenever is convenient for you Regression (Normal Equation) - FreeSurfer Volumes => Regression - FreeSurfer Volumes Decentralized DFNC => Decentralized Dynamic Functional Connectivity (DDFNC) Pipeline fMRI Preprocess => fMRI Preprocessing VBM Preprocessor => Voxel-Based Morphometry (VBM)

Just leave Multishot regression as is for now.

everner commented 4 years ago

Also, please add the Group ICA computation as "Group Independent Component Analysis (ICA)".

rssk commented 3 years ago

Closed by #808