-
Notifications
You must be signed in to change notification settings - Fork 523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TypeError in ridgeWeightedReg.mjs #265
Comments
Thank you! You may have identified a key bug. I'll look into this soon. |
@larsbonczek can you also say under what conditions the exception was thrown? Like had the code been running for a while, or was this a certain configuration? |
This error occurred in an online experiment built using PCIbex. The exception is usually thrown part-way into the experiment (which is quite long). I took a look at PCIbex' implementation of the eye tracking element and realized that it generates a lot of data points during the calibration process (it emulates a click event multiple times per second). I verified this by checking the number of calibration points saved into the browser's IndexedDB by webGazer. One run of the calibration process generates 602 data points. After that, another calibration step is performed after each step of the experiment. When the error occured, I checked the IndexedDB again and saw that the stored list had reached 700 data points, which matches the window size mentioned above. I think that due to the large amount of data points that is generated during the calibration process, the data window containing the calibration points fills up quite quickly during the experiment, which triggers the above exception. Testing this hypothesis is difficult, since PCIbex automatically triggers a re-calibration if the calibration score falls below a certain threshold during the experiment. Before this re-calibration, the data window is cleared, thus preventing the error from occurring. The error only occurs if no re-calibration is required for a sufficient number of steps, which is a rare occurrence. |
I think you're exactly right. weightedXArray and weightedYArray data are being accessed with the trueIndex index in lines 66-67, but lines 68-69 and even 72-73 treat it like a regular array, so there's a mismatch between the index used, once the DataWindow size is about 700 (the default window size) The bug seems to be introduced in an old commit but the pre-commit code also seems wrong to me (it's push()ing to an array that was already initialized to the correct length) From just my reading, the pull request seems perfect to me, but someone should test it. An easy way to test might be to reduce the default window size from 700 to 10, and see if the fix does indeed work. |
I'm not familiar with webGazer, so I don't think that I am the right person to test this. Would you be willing to give it a try? |
I'm going to look this over a little bit, maybe give it a brief test, but the PR seems pretty harmless either way. @alexpapster said she'd be able to test it a bit more thoroughly later but I think it's fine to merge in the meantime. Thanks again for investigating Lars. |
We have encountered this error
in line 68 of ridgeWeightedReg.mjs (see below). I haven't debugged the problem carefully yet, but looking at the surrounding code, I feel like I might have spotted the mistake:
WebGazer/src/ridgeWeightedReg.mjs
Lines 50 to 70 in 034974b
In line 68 we access a property of
weightedXArray[i]
, which can only be defined ifi === trueIndex
.trueIndex
comes from line 57:WebGazer/src/ridgeWeightedReg.mjs
Line 57 in 034974b
Since
this.eyeFeaturesClicks
is a DataWindow,i === trueIndex
will only hold ifthis.eyeFeaturesClicks
is not full yet. As soon as the data window is full,trueIndex
andi
will be different, thus the TypeError occurs.Suggested solution
I'm not certain, but I think line 66 should say
weightedXArray[i]
instead ofweightedXArray[trueIndex]
, soweightedXArray[0]
contains the oldest value. This way the weighting (lowest weight fori=0
, highest fori=len-1
) would make sense, too. The same goes for line 67.The text was updated successfully, but these errors were encountered: