BUNPC / Homer3

MATLAB application for fNIRS data processing and visualization
92 stars 54 forks source link

Documentation update #113

Closed Horschig closed 1 year ago

Horschig commented 2 years ago

Hi all,

as discussed with Vijay, we have updated the documentation of the function listed in https://github.com/BUNPC/Homer3/wiki/Processing-Functions We fixed some mistake and completed some others, so that users can get a better understanding of the parameters that the function is using. This was a major pain to us, because sometimes we had to dig deep in the code to find out how a certain parameter should actually be specified. This PR should be a remedy for at least those functions listed in the wiki.

Horschig commented 2 years ago

Some other comments we have:

hmr_MotionCorrectPCA.m script issues: The description says that the nSV could be an integer with the number of principal components to remove. This is not how the function works – the function checks if nSV is in the range of (0, 1) and only filters the principle components then. In the preprocessing stream UI, nSV can be provided as a fraction of the variance. When it is provided as a fraction with 2 decimal positions, e.g., 0.97, it is rounded to the first decimal point which means that the lowest possible value which will lead to some change is 0.9. I think this is suboptimal. For example, for a cumulative sum of eigenvalues in a 4 channel device are the following

0.828324335440192
0.984492047827710
0.999380069159404
1.00000000000000

Therefore, whether you take 0.85 or 0.99 as a threshold, it makes a big difference, but in the toolbox, it would be 0.9 and 1.

hmrR_MotionCorrectPCArecurse.m – In the description it says that the max number of iterations used by Yucel is 5, but at least in the paper from 2014, it is 3.

In hmrR_MotionCorrectSpline.m script, the input for active channels is mlAct, which might be outdated because other functions derive mlAct = mlActMan{iBlk} & mlActAuto{iBlk}; Generally, most of the correction functions do not take into account the mlActMan. Not sure if that is the intention. I think in all cases there should be a union derived from mlActMan & mlActAuto

data_dod = hmrR_MotionCorrectRLOESS(data_dod, span, turnon) Is not actually applied on motion artifacts – the function is applied on full time-series data. It does not take tIncAuto or tIncAutoCh variables marking the motion artefact segments into account. I am not sure it should be called MotionCorrect. (edited)

vijayiyer05 commented 2 years ago

Glad to see the convergence of the inline & wiki documentation; and excellent to see all the spot improvements contributed.

Horschig commented 2 years ago

To ends here:

% convert to dod dod = DataClass().empty(); for ii=1:length(intensity) @@ -30,3 +35,15 @@ dod(ii).SetMl(intensity(ii).GetMl()); dod(ii).SetDataTypeDod(); end end

Hi, do you mean "two ends", meaning we should add an "end" there? It's not really something we looked at, but sure, we could do that (if that is what you meant).

mayucel commented 2 years ago

Hi, there is one for loop, so I did not get why there are two "ends" rather than one. Maybe I am missing something.

Horschig commented 2 years ago

I can only see one end:

image

So we thought you maybe want a second added (to indicate the end of the function)?

mayucel commented 2 years ago

Perfect. I could not track back where I copied the above from. Will close the request change. Thx.

Horschig commented 1 year ago

Hi, this branch here has never been merged. Maybe you guys were not aware that I/we are not allowed to do that, so you guys have to do it. There are conflicts now. I resolved them in another branch and will create a new PR.

Horschig commented 1 year ago

The new PR is https://github.com/BUNPC/Homer3/pull/184