brownhci / WebGazer

WebGazer.js: Scalable Webcam EyeTracking Using User Interactions
https://webgazer.cs.brown.edu
Other
3.54k stars 536 forks source link

TypeError in ridgeWeightedReg.mjs #265

Closed larsbonczek closed 2 years ago

larsbonczek commented 2 years ago

We have encountered this error

TypeError: Cannot read properties of undefined (reading '0')

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:

https://github.com/brownhci/WebGazer/blob/034974b52375f78ac57a09ed377c052ba0c1dad8/src/ridgeWeightedReg.mjs#L50-L70

In line 68 we access a property of weightedXArray[i], which can only be defined if i === trueIndex. trueIndex comes from line 57: https://github.com/brownhci/WebGazer/blob/034974b52375f78ac57a09ed377c052ba0c1dad8/src/ridgeWeightedReg.mjs#L57

Since this.eyeFeaturesClicks is a DataWindow, i === trueIndex will only hold if this.eyeFeaturesClicks is not full yet. As soon as the data window is full, trueIndex and i will be different, thus the TypeError occurs.

Suggested solution

I'm not certain, but I think line 66 should say weightedXArray[i] instead of weightedXArray[trueIndex], so weightedXArray[0] contains the oldest value. This way the weighting (lowest weight for i=0, highest for i=len-1) would make sense, too. The same goes for line 67.

jeffhuang commented 2 years ago

Thank you! You may have identified a key bug. I'll look into this soon.

jeffhuang commented 2 years ago

@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?

larsbonczek commented 2 years ago

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.

jeffhuang commented 2 years ago

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.

larsbonczek commented 2 years ago

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?

jeffhuang commented 2 years ago

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.