Homer3
Homer3 copied to clipboard
BUNPC
Documentation update
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.
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)
Glad to see the convergence of the inline & wiki documentation; and excellent to see all the spot improvements contributed.
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).
Hi, there is one for loop, so I did not get why there are two "ends" rather than one. Maybe I am missing something.
I can only see one end:
So we thought you maybe want a second added (to indicate the end of the function)?
Perfect. I could not track back where I copied the above from. Will close the request change. Thx.
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.