search

tramarobin / fctSnPM

Using spm1d package (v.0.4.3), compute anova and post-hoc tests from anova1 to anova3rm, with a non-parametric approach (permutation tests)
GNU General Public License v3.0
2 stars 2 forks source link

JOSS review questions #19

Closed nicholst closed 3 years ago

nicholst commented 3 years ago

Hi,

I'm providing a much belated review for JOSS. Please consider the following issues...

README.md:

Post-hoc tests with Bonferonni correction are only approximate.

Can you clarify if this is the usual "Bonferonni is conservative", or something more fundamental relating to a post-hoc test being viewed only after a significant main effect. Not asking for a huge exposition, but just e.g. "... Bonferonni correction are conservative" or "... Bonferonni correction are only approximate because..."

Typos/spelling errors: please run a spell check on the documentation (e.g. "This function synthetises" ->"This function synthesizes"; or "results of teh statistical analysis")

.\fctSPM\Examples\D1_ANOVA2_1rm.m is a windows-centric path specification and Matlab doesn't support executing a script with a path. Presumably it would be better to say "The output of D1_ANOVA2_1rm.m in the Examples folder is as follows" (or something).

Input types: I must say I like the clear designation of input type in the documentation of inputs, e.g.

savedir is the path ... might be overwritten @ischar."

but it is done inconsistently. I.e. the main inputs don't have these end-of-sentence tags (and IT doesn't have any), and they are typset inconsistently when given (sometimes preceeded by ".", "..." or just space). Maybe just integrate them into the prose description or consistently add them in parentheses at the end (char).

iterations: As permutation tests are used here, I kept search the documentation and code: How many permutations are used? Eventually I figured out that (please correct me if not!) that "iterations" is supposed to be read as "permutations". Iterations is usually understood in the context of optimisation, iteratively improving an objective function. I can't expect you to replace a term that appears in 132 lines of code, but please, at least at the first reference of "iteration" in the documentation please clarify this is the number of permutations.

fctSPM.m & fctSPMS.m

"Information" is always singular, but multiple times it is used as "Informations"; also, the lines starting "Please read README.md" have a strange nonprinting character before "Please".

Also very minor, the hypen in the Nichols & Holmes reference isn't plain ASCII; its better to replace "–" with vanilla "-".

tramarobin commented 3 years ago

Thank you for all your comment @nicholst

Can you clarify if this is the usual "Bonferonni is conservative", or something more fundamental relating to a post-hoc test being viewed only after a significant main effect. Not asking for a huge exposition, but just e.g. "... Bonferonni correction are conservative" or "... Bonferonni correction are only approximate because..."

This was clarified and I added a link to the spm1d documentation to find all the required informations.

Typos/spelling errors: please run a spell check on the documentation (e.g. "This function synthetises" ->"This function synthesizes"; or "results of teh statistical analysis")

I ran a spell check, I think it is ok now.

.\fctSPM\Examples\D1_ANOVA2_1rm.m is a windows-centric path specification and Matlab doesn't support executing a script with a path. Presumably it would be better to say "The output of D1_ANOVA2_1rm.m in the Examples folder is as follows" (or something).

This was modified accordingly.

Input types: I must say I like the clear designation of input type in the documentation of inputs, e.g. savedir is the path ... might be overwritten @ischar." but it is done inconsistently. I.e. the main inputs don't have these end-of-sentence tags (and IT doesn't have any), and they are typset inconsistently when given (sometimes preceeded by ".", "..." or just space). Maybe just integrate them into the prose description or consistently add them in parentheses at the end (char).

This section of the README file was modified accordingly.

iterations: As permutation tests are used here, I kept search the documentation and code: How many permutations are used? Eventually I figured out that (please correct me if not!) that "iterations" is supposed to be read as "permutations". Iterations is usually understood in the context of optimisation, iteratively improving an objective function. I can't expect you to replace a term that appears in 132 lines of code, but please, at least at the first reference of "iteration" in the documentation please clarify this is the number of permutations.

It is indeed more logical to call these variable permutations. Iteration was modified as permutation, and IT as Perm. It was modified in the documentation and scripts.

"Information" is always singular, but multiple times it is used as "Informations"; also, the lines starting "Please read README.md" have a strange nonprinting character before "Please".

This was modified.

Also very minor, the hypen in the Nichols & Holmes reference isn't plain ASCII; its better to replace "�" with vanilla "-".

I think I corrected it but I'm not sure...

nicholst commented 3 years ago

Thanks for the quick response @tramarobin.

Thanks for the updates. It all looks good... all that's left is trivial:

You corrected the Nichols & Holmes 'dash' in the 2nd but not the first instance in fctSPM*.m (look after "please cite for permutation").

Also, totally trivial, but might as well fix it: Mostly you have references to Unix-style paths and "/" as path separator. But I just noticed ...

fctSPM.m:% Examples are in ...\fctSPM\Examples
fctSPM.m:% see .\fctSPM\Examples for help
fctSPMS.m:% Examples are in ...\fctSPM\Examples
fctSPMS.m:% see .\fctSPM\Examples for help
saveNplot.m:% see .\fctSPM\Examples for help

I would simply avoid this by "Examples are in the Examples directory" ... there can be no confusion about that I think.

OTW, good to go!

tramarobin commented 3 years ago

You corrected the Nichols & Holmes 'dash' in the 2nd but not the first instance in fctSPM*.m (look after "please cite for permutation").

Oh sorry I forgot this one. It should be ok now.

I would simply avoid this by "Examples are in the Examples directory" ... there can be no confusion about that I think.

I modified this line as follow : You can find different scripts creating output for 1D and 2D data in ...\fctSPM\Examples

nicholst commented 3 years ago

Ok… but my other point was that, to my Linux eyes, I stumble over a path separator of “\” and I saw “/“ elsewhere somewhere in the code (I think), and so suggested a formulation that avoided path separators altogether…. But honestly I’m not that fussed…

tramarobin commented 3 years ago

Ok, sorry, I did not understood your remark on this point, but I do now.

If it works on Linux too (I only tested on Windows and Mac), It would be better to let it as it is I think. I did not now the fullfile function until now, and although it would be a better formulation, it would add nothing if it already works fine (and also save me a lot of time as many files are created with different paths...)

I checked the different lines of code and change some '\' in '/'. I hope it wont be any left.

tramarobin commented 3 years ago

@nicholst, can we close this issue ?

nicholst commented 3 years ago

Yes yes!