search

UMPsychMethodsCore / MethodsCore

All of the projects that the methods core develops, combined into one repository!
7 stars 0 forks source link

First level alpha #337

Closed heffjos closed 8 years ago

heffjos commented 10 years ago

Here are the major updates to the first level:

  1. FirstLevel_mc_central.m has been split into several functions. Hopefully, the logic is reasonable and the data structures are well documented within the code.
  2. The spm batch system is used to run first levels.
  3. More error checks have been inserted. Error messages should be more verbose.
  4. Parametric regressors must be assigned to a condition now. Polynomial orders for parametric regressors can now be specified in the ParametricList variable.
  5. Derivative and polynomial orders can be specified for regressor files in the RegFilesTemplate variable. Polynomial orders are applied to the regressors and its derivatives.
  6. Contrasts vector lengths as specified in the ContrastList should be properly checked and errors out if a contrast row is incorrect. This is dependent on the basis function used for the model and parametric regressors.
  7. A text file named AllSubjects.txt is written in the log directory which summarizes all subject information.
  8. The basis function used for the model is specified by the variable Basis. Currently only the canonical hemodynamic response and finite impulse response functions are available.
  9. The variable HrfDerivative is only used when the 'hrf' basis function is used. It controls the number of derivatives to model for the 'hrf' basis function.
  10. The variable FirDuration is only used when 'fir' basis functions are used. It specifies the 'fir' length.
  11. The variable FirBins is only used when 'fir' basis functions are used. It specifies the 'fir' order.
  12. The variable FirDoContrasts will automatically create a contrast for each bin for all conditions. The contrasts listed in ContrastList will still be created.
  13. CompCor can now be performed with the CompCorTemplate variable. The files generated by pcafMRI are used to perform CompCor.
  14. NumScan variable has been changed to the VolumeSpecifier variable. The VolumeSpecifier variable can be used the trim volumes from the beginning and end of a run.

Master Datafile Changes:

  1. Commented lines are denoted by '#' at the beginning. These lines are ignored when reading the data file.
  2. Subject numbers have been removed; instead, subject data is found by the subject name as listed in the SubjDir variable.
  3. The mixed data file scheme has been implemented. Lines containing pointers to other files are denoted by '@' at the beginning.
  4. Strings can be present in data files as long as they are not in columns listed in the template file.
  5. The update to data files are documented in the first level help pdf.

Some subtle updates:

  1. CondColumn, TimeColumn, and DurationColumn must have the same number of elements.
  2. If using multiple condition columns, the parametric regressor will be used with every condtion number listed with that parametric regressor.
  3. Currently regressors added from CompCor are not considered when creating contrasts.
  4. The run numbers listed in the data files must be chosen so that they correspond with the runs to include vector in the SubjDir variable.

Does anyone have any comments on the changes? Also considering the number of changes I think it is best someone else run 1 or 2 tests to confirm it is working as intended before it is merged into public.

mangstad commented 10 years ago

Few quick comments from the email:

Regarding this:

  1. Currently regressors added from CompCor are not considered when creating contrasts

Is there a reason this couldn't just be implemented through the same RegFileTemplates variable as others like motion/etc? They're already dealt with automatically for contrasts, and they use your parsing function to read, so as long as the output format from pcafMRI is compatible with that, they could just be treated the same as other regressors. I think it's unnecessary and potentially confusing for users if we start treating some regressors in different ways.

We will definitely want to do testing. I know that when I was trying to use a previous version of this to run some FIR stuff, the sandboxing was completely broken in several places. I made some quick hack fixes at the time just to get the analysis to run. I'll check to see whether they're reasonably robust or need to be improved, and see if it's still easily mergeable with this current version.

Electronic Mail is not secure, may not be read every day, and should not be used for urgent or sensitive issues

heffjos commented 10 years ago

@mangstad

I did not implement CompCor through RegFileTemplate, so that we could pick a minimum number of components to explain a selected variance. This could cause the number of components used to be different for each subject. If we do not want this option, then CompCor should be able to be used in the RegFileTemplate variable. The one thing about this is the first column from the csv file generated by pcafMRI is the global mean of the mask whereas the subsequent columns are the ordered principle components. We could implement an option where the user specifies a range of regressors to use from the file.

Sandboxing definitely needs testing. That is the only feature I could not test on my own, because the Zubieta cluster does not have a sandbox.

dankessler commented 10 years ago

I recall @heffjos brought up at the last MC meeting that there was some ugliness with this commit, as it looks like it is altering 518 files with tens of thousands of additions and deletions. From comparing which branches have been merged to each of these tips, it looks like FirstLevel_alpha also absorbed ConnTool_alpha sometime earlier. Is this a required merge, or can I revert that to make this a bit more manageable?

mangstad commented 10 years ago

While I'm not 100% positive, I can't think of a reason why they would need to be merged. I'm assuming no helpful commit messages on the merge that brought ConnTool in?

Electronic Mail is not secure, may not be read every day, and should not be used for urgent or sensitive issues

heffjos commented 10 years ago

I had to merge it only to successfully backport public into FirstLevel_alpha without any merge conflicts.

dankessler commented 10 years ago

Wanted to bring this back to life. It's been a little while so let me make sure I recall things correctly.

So, the collected changes include all the work that was done in FirstLevel/BatchSystem, in addition to all of the backport stuff, right?

rcwelsh commented 10 years ago

Yes, we need to bring this back to life.

-Robert

From: Daniel A Kessler notifications@github.com<mailto:notifications@github.com> Reply-To: UMPsychMethodsCore/MethodsCore reply@reply.github.com<mailto:reply@reply.github.com> Date: Monday, May 12, 2014 11:35 AM To: UMPsychMethodsCore/MethodsCore MethodsCore@noreply.github.com<mailto:MethodsCore@noreply.github.com> Subject: Re: [MethodsCore] First level alpha (#337)

Wanted to bring this back to life. It's been a little while so let me make sure I recall things correctly.

So, the collected changes include all the work that was done in FirstLevel/BatchSystem, in addition to all of the backport stuff, right?

— Reply to this email directly or view it on GitHubhttps://github.com/UMPsychMethodsCore/MethodsCore/pull/337#issuecomment-42847642.

Electronic Mail is not secure, may not be read every day, and should not be used for urgent or sensitive issues

heffjos commented 10 years ago

So, the collected changes include all the work that was done in FirstLevel/BatchSystem, in addition to all of the backport stuff, right?

Yes, the changes include all work that was done in FirstLeve/BatchSystem in addition to all the backport stuff. There were some issues getting a clean merge when backporting public. I was able to get a clean merge by reverting 2 commits, except the 'som' folder popped back in along with the 'ConnTool'.

dankessler commented 10 years ago

To make this simpler, I've opened a pull request to backport public directly into FirstLevel_beta. That will make this diff look MUCH simpler. If you want to see what backporting does, look at #346, which I am about to go ahead and accept, and that will make this diff look MUCH simpler.

Hopefully @mangstad can now take a crack at testing all of the FirstLevel improvements.

heffjos commented 10 years ago

I feel this is ready for testing again. I updated some code so that it hopefully can do sandboxing. I also took @mangstad advice so that CompCor is implemented through the variable RegFilesTemplate. I was thinking we could give Daniel W a copy of this and let him beta test it.

I had updated the documentation awhile ago, but I will review it and make sure it is up to date.

dankessler commented 8 years ago

So this is the last open pull request before I can get to work rolling alphas into betas and taking away the alphas. Since Joe wrote it, I assume we want one other developer to review it, so it's either me or @mangstad

Since @mangstad has a bit more familiarity with the FirstLevel scripts since the original drafts were his work, I propose he do it, but I can pinch hit if there's some reason he can't or is unavailable.

mangstad commented 8 years ago

Will do. Though minor comment, I think we decided to get rid of the alphas, not the betas.

dankessler commented 8 years ago

Yes, edited my comment above to be accurate: rolling alphas into betas then ditching the alphas