(Fix) Error velocity QC should be & not |

[evacougnon]

Originally submitted by @BecCowley :

Should have an & to find error velocity values that are between the two extremes.

Supersedes #756

[evacougnon]

@BecCowley do you know why the following change was made? From

% Run QC
iPass = abs(erv) <= err_vel;

to

% Run QC NEED TO edit this as it should be the mean +/- the threshold.
ervm = nanmean(erv(:));
iPass = erv >= ervm - err_vel | erv <= ervm + err_vel;

(link to the corresponding commit here)

This was committed in Aug 2020 and included in the last release (20 Apr 2021) but the wiki wasn't updated. Based on the current information and documents available in the wiki this change does not make sense to me. The references I have only use a fixed threshold. I'm probably missing out a point. Let me know if you have more information about this update. Before to merge this PR I would like to make sure the wiki is up-to-date. Thanks

[BecCowley]

@evacougnon. Yes, I put this in because I noticed that error velocity is very rarely centered around zero. If we use an absolute threshold, we might be biasing the QC process. However, I'm not convinced about this change yet. My thought is we park it for now, leave the code as it is. I think it makes very little difference to the final results anyway.

[evacougnon]

@BecCowley , thanks for the explanation.

I have 2 concerns with the current version of the code: iPass = erv >= ervm - err_vel | erv <= ervm + err_vel;

• does not flag anything as fail
• does not match what we have in the wiki

If we park this fix for now, I'd be keen to revert to the original code iPass = abs(erv) <= err_vel; until we fix it

[BecCowley]

@evacougnon, the code will flag anything outside the mean +/- the threshold as fail (I think it does that in a later line?). However, please revert to the original code.

[hugo-sardi]

Hello, After the discussion yesterday and looking at the code I think I found the trouble here:

The problem is not the & or | operator, It is the sign.

The expression should be:

iPass = erv >= ervm + err_vel | erv <= ervm - err_vel;

I don't know about you, but when I read the original code I saw the expression above instead of the actual code. I just tested it this morning and I went straight and wrote the code myself to mark everything distant around the mean. Then I went back to the code and saw that the sign error.

A typical case of being more explicit helps a lot:

above_thres = erv >= ervm + err_vel;
below_thres = erv <= ervm - err_vel;
iFail = below_thres | above_thres;

I believe the above code could hardly pass a code session (or a review) if it was written the opposite way (above_thres = erv >= ervm - err_vel).

[sspagnol]

Just checking I'm not missing something, is this equivalent to

iPass = abs(ervm - erv) >= err_vel;

Don't know if one form or another is more efficient.

[hugo-sardi]

yeap

[BecCowley]

@hugo-sardi, Your example would give anything outside the mean +/- the threshold. Therefore, it should be: iFail = below_thres | above_thres;

Yes?

[hugo-sardi]

@BecCowley - yeap - the snipped above is to be assign to iFail. I will correct the comment to avoid confusion