New denoising modules and miscellaneous fixes - #262

1. globalmean: I prefer avoiding uppercase on stream names (i.e. globalmean instead of GLOBALMEAN). Also AFAIR, we decided using uppercase is module names only for tools (e.g. FLIRT).
2. brainmask: How is aamod_brainmask different from aamod_mask_segment? Shall we unify them?
3. firstlevel_contrast: What do you mean "apply string contrast across all sessions as a whole"? Is not it the same as sameforallsessions?
4. halt: What is the use case?
5. reslice2epi: How is aamod_reslice2epi different from aamod_denorm.m?
6. segment8_noT2: Why is it necessary? I thought aamod_segment8 is adaptive.
7. waveletdespike: I think using aas_stream_has_contents to check the presence of the mask is a better solution than an explicit option for "no mask".
8. wdsClass: Considering that the official abbreviation of the toolbox is BWT (see lab website and paper), I suggest bwtClass as interface.
9. developer/testscripts/spm_2021Oct22.ps: I think it is an accidental addition.
10. washu_surfacerender: Do you want to replace/remove external/surfacerender?
11. extrafunctions/*: If you (plan to) use these functions only once, then I suggest converting them into subfunctions to reduce the number of (spurious) files and their chance of becoming "orphan".

1. globalmean: I prefer avoiding uppercase on stream names (i.e. globalmean instead of GLOBALMEAN). Also AFAIR, we decided using uppercase in module names only for tools (e.g. FLIRT).

IIRC, I used uppercase because global mean is used with DVARS and FD when doing frame censoring and DVARS and FD are universally uppercase in the literature. But aamod_globalmean is fine by me. Should I also change aamod_dvars instead of aamod_DVARS and stream fd instead of FD?

2. brainmask: How is aamod_brainmask different from aamod_mask_segment? Shall we unify them?

aamod_brainmask does something that aamod_mask_segment does not: it makes the input and output streams absolutely unambiguous. This is going to be the next big debate in aa, whether we have buffet style modules (one header with renameable streams) or menu style modules (collections of aliased headers, each with explicitly defined inputs and outputs). I prefer menu-style modules because they clearly document the analysis, which is why I wrote aamod_brainmask rather than using aamod_mask_segment.

3. firstlevel_contrast: What do you mean "apply string contrast across all sessions as a whole"? Is not it the same as sameforallsessions?

When using scrubbing, there's no way to know a priori how many columns will be added to the design matrix or in what sessions they will appear. As such, you can't predefine a fixed contrast vector using sameforallsessions (or even uniquebysession). The new contrast option I added will search the design matrix and generate the proper contrast vector for you. That's what I meant by my cryptic "apply string contrast across all sessions as a whole".

4. halt: What is the use case?

aamod_halt forces Matlab to halt execution. It's not a big deal -- it's just something I've found useful. You can add / remove / move one module rather than deleting/undeleting big chunks of a tasklist (and the scheduler will still check your stream dependencies). I wrote a little blurb on it in my tutorial appendix but not sure that's in the version I emailed around.

5. reslice2epi: How is aamod_reslice2epi different from aamod_denorm.m?

I would say the main difference is I understand what aamod_reslice2epi does: it reslices a file to the epi. OTOH, I'm still unclear on what aamod_denorm does, even after reading the header.

6. segment8_noT2: Why is it necessary? I thought aamod_segment8 is adaptive.

aamod_segment8 will use the T2 if it exists; the problem is aamod_segment8 will ALWAYS use the T2 if it exists. Sometimes we want aamod_segment8 to ignore the T2 even if it exists. Ergo, aamod_segment8_noT2.xml.

7. waveletdespike: I think using aas_stream_has_contents to check the presence of the mask is a better solution than an explicit option for "no mask".

The idea is to let the user control whether to do masking regardless of whether a mask exists or not. (If a user tries to use a mask that doesn't exists, that's on them).

8 wdsClass: Considering that the official abbreviation of the toolbox is BWT (see lab website and paper), I suggest bwtClass as interface.

I will change.

9. developer/testscripts/spm_2021Oct22.ps: I think it is an accidental addition.

Dang it. SPM snuck this into my repo during testing. I will delete.

Aside: Is there a way to globally turn off .ps creation in SPM? These postscript files are useless.

10. washu_surfacerender: Do you want to replace/remove external/surfacerender?

No. This is an alternate renderer developed at WashU that we like but maybe other people won't (so they should be able to use the old renderer).

(Also, this renderer is used in a new FSL randomise module I wrote that wasn't intended to be part of this PR. (The code has been tested -- I just didn't provide example and aa_test scripts for it yet. I'll do that as a separate PR).

11. extrafunctions/*: If you (plan to) use these functions only once, then I suggest converting them into subfunctions to reduce the number of (spurious) files and their chance of becoming "orphan".

I don't understand this comment. Which functions?

1. lowercase stream names
   Yes, please, change globalmean, dvars, and fd both in module names and stream names.

2. brainmask:
   This question was rather about the codebase (m-file) rather than the headers. Would you consider aamod_mask_segment with aliased headers with explicitly defined inputs and outputs? It seems it would reduce the number of files because aamod_mask_segment gives you all three segmentations.

3. firstlevel_contrast:
   How do you specify this new contrast? I have checked the scrubbing test, but I have not seen the specification.

4. reslice2epi:
   aamod_denorm transforms an image from MNI to subject's T1 space by applying the inverse of the normalisation transformation. It also has a decoregextended option to transform the denormalised image to the session's space (dwi or epi). It can handle both freesurfer and SPM's unified normalisation. Including DARTEL is the next step. At the moment it has a separate module aamod_dartel_denorm.

5. waveletdespike:
   it makes sense, then. I thought you want to use -1 to indicate the absence of a mask. Can you update the in-code documentation (change 'no mask' to 'do not mask')? Also, there is a typo there ('- 0-1: probability thteshold for spm brainmask').
   You also set the default behaviour to -1. Was that intentional? Is not it recommended to mask? Our default parameters are supposed to represent the best practice.

6. developer/testscripts/spm_2021Oct22.ps:
   I am not sure whether you can turn it off, but you can add '.ps' to the .gitignore, so it does not end up in the repo.

7. washu_surfacerender: Do you want to replace/remove external/surfacerender?
   The current surfacerender is not used by any module. I think we should remove it; especially if we are going to have a new one.

8. extrafunctions/*:
   I meant any new function you now add to extrafunctions. If you (plan to) use them only in one place, then I suggest converting them into subfunctions to reduce the number of (spurious) files and their chance of becoming "orphan".

Yes, please, change globalmean, dvars, and fd both in module names and stream names.

Done.

This question was rather about the codebase (m-file) rather than the headers. Would you consider aamod_mask_segment with aliased headers with explicitly defined inputs and outputs? It seems it would reduce the number of files because aamod_mask_segment gives you all three segmentations.

Yes, here use of aamod_brainmask does indeed reduce the number of files generated in the analysis by a factor of 3, but I'm not advocating deleting aamod_mask_segment. Again, it comes down to user preference. I prefer specifically named modules (i.e, collections of aliased xml headers) with specifically named input and output streams. Others may prefer to use one generic module with renameable streams. The latter appears to be how aa usually worked in the past, but we're a big enough tent to support both programming styles.

How do you specify this new contrast? I have checked the scrubbing test, but I have not seen the specification.

None of the datasets we currently use for testing require this feature. It's really only necessary when you are defining a contrast across sessions that have different regressors plus an unknown number of nuisance regressors (cf. scrubbing). It simply allows you to use string defs with uniquebysession (which currently only takes a vector def).

However, I did greatly expand the help blurb at the top of aas_addcontrast to now include examples of how to use my new contrast def (and most of the other common contrast definition techniques). Hopefully that will clarify things.

aamod_denorm transforms an image from MNI to subject's T1 space...

See my defense of aamod_brainmask above. I prefer specific named modules to generic modules with renameable streams.

it makes sense, then. I thought you want to use -1 to indicate the absence of a mask. Can you update the in-code documentation (change 'no mask' to 'do not mask')? Also, there is a typo there ('- 0-1: probability threshold for spm brainmask').

I edited the header to clarify.

You also set the default behaviour to -1. Was that intentional? Is not it recommended to mask? Our default parameters are supposed to represent the best practice.

"no masking" gave us the best results when using wavelet despiking. YMMV.

I am not sure whether you can turn it off, but you can add '.ps' to the .gitignore, so it does not end up in the repo.

Added spm*.ps to .gitignore

The current surfacerender is not used by any module. I think we should remove it; especially if we are going to have a new one.

I meant any new function you now add to extrafunctions. If you (plan to) use them only in one place, then I suggest converting them into subfunctions to reduce the number of (spurious) files and their chance of becoming "orphan".

I agree with culling orphans or folding them into subfunctions. However, just because a function isn't called from a module or only one module, does that mean it isn't used or useful elsewhere? Perhaps some of these functions are called in user code outside of the repo or provide functionality that can be invoked from the command line?

aamod_brainmask

Yes, here use of aamod_brainmask does indeed reduce the number of files generated in the analysis by a factor of 3,
I said it in the favour of the aamod_mask_segment because it already generates masks for all (three) tissues, while you need three instances of aamod_brainmask to get the same.

reslice2epi
So, aamod_denorm seems to do the same then, and you can just create specific headers for it to implement "menu style". In fact, aamod_denorm is used in "menu style" and aamod_denorm.xml does not exist.

washu_surfacerender and extrafunctions

Perhaps some of these functions are called in user code outside of the repo or provide functionality that can be invoked from the command line?
I would avoid turning aa into a collection of non-aa tools. My preference is to use extrafunctions only for subfunctions required/called by aa functions. We also have aa_tools to bundle "useful" functions that are optional and/or may be used to provide specific functionalities, however, only for aa or results from aa analyses.
For other functions or tools, I recommend separate repositories, which can be separately maintained.

+1: CI
I have implemented CI using GitHub actions. Please see the corresponding discussion. I have also updated/reverted test functions to fix the bugs. If you merge the central master into your branch, then your PR can also run the checks "automagically" (see my PR, as an example).
I suggest resolving the conflicts by directly copying the updated files to your branch because GitHub's conflict resolution would merge your branch into the central master in the process. I learned in a hard way :(, and I had to "rewrite the history" of the central master.

I have made al of the changes to my code that I'm going to. So either approve my PR or delete it. I don't have time to spend another month going round and round on this.

Your call.

Do you disagree or have you no time?

I am afraid merging is not even possible without resolving the conflicts, so that is the minimum requirement.

Then, if you agree with my comments but simply have no time or motivation to adopt them, I can do it after merging your PR.

I am afraid merging is not even possible without resolving the conflicts, so that is the minimum requirement.

How odd: 1) I submit code, 2) other people break my code, 3) I'm expected for fix my code.

Then, if you agree with my comments but simply have no time or motivation to adopt them, I can do it after merging your PR.

You will do no such thing. Your implementation is A BAD DESIGN. It is THE WRONG APPROACH. It is CONFUSING TO USERS. It is PRONE TO ERROR.

How much clearer can I make this?

I implemented a framework that allows automatic testing (CI) and eliminates the need of downloading the data and checking out and testing the PR locally on your system. How bad design it is?

Your code is not broken but not compatible with the updated design (CI). Do you disagree with the design (i.e. implementing CI)?

Last offer: submit the PR to a new branch. I merge it, I adapt to the CI framework, and I merge it into the master. Then, I add the changes you did not argue with.
You can decide whether and at which stage (i.e. after CI or after all changes) you want to re-merge these changes to your repo.

I think your developments are great stuff, and I am happy to run the extra miles to merge them.