Skip to content
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

Multiple mistakes appeared in the code while I was trying to refactor it and enforce type checking. Can someone just answer those questions? #337

Open
Sharcoux opened this issue Mar 15, 2024 · 11 comments

Comments

@Sharcoux
Copy link

Sharcoux commented Mar 15, 2024

Ok, here are the list of problems I noticed in the code:

  • Code duplication
  • Dead code
  • Type confusions
  • Logic differences between some of the duplicates
  • Some broken logic
  • @ts-check annotation could have saved lives
  • Generated files are being tracked by git

I'm trying to solve all of that, but I need some help with the following issues I'm not sure about how to solve:

1st problem

y[i] = [y[i]];

y[i] = [y[i]]: this will transform a number[][] array into a number[][][] array. I would be surprised if that was intentional, provided the comment above. My guess is that the intended code was:

const line = y[0]
for (let i = 0; i<y.length; i++) {
  y[i] = [line[i]]
}

2nd problem

var y = sub(z, mult(this.H, X_p)); // This is the measurement error (between what we expect and the actual value)

z is supposed to be of type number[] while the sub method expects a parameter of type number[][]. In another file, I noticed the exact same code with that single difference: there was that extra line before:

    const transposedZ = transpose([z])
    const y = sub(transposedZ, mult(this.H, xP)) // This is the measurement error (between what we expect and the actual value)

I guess that this is the correct version?

3rd problem

m_Coefficients[i] = solution[i];

mCoefficients[i] = solution[i]: mCoefficients is of type number[] but solution is of type number[][]. I have no idea how this is supposed to be solved. Should I just take solution[i][i]?

4th problem

if (m_Coefficients.length*n !== m_Coefficients.length){

if (mCoefficients.length * n !== mCoefficients.length): This can only be true if n === 1. Is it what we want to test instead?

Anyway, the line above that define the value of n is completely stupid: const n = (mCoefficients.length !== 0 ? mCoefficients.length / mCoefficients.length : 0)

This is equivalent to mCoefficients.length ? 1 : 0. In short, those 2 lines could be simplified with if(mCoefficients.length). However, there is absolutely no relation with Array length being a multiple of m...

5th problem

var H = [ [1, 0, 0, 0, 0, 0],

H is being declared twice. Which one is the correct value?

6th problem

x += smoothingVals.get(d).x;

d is of type string. This will lead to really problematic consequences in the get and getTrueIndex methods

7th problem

init(debugVideoLoc);

debugVideoLoc has to be a MediaStream, because we call getTracks() on it. But it is a string, here...

@jeffhuang
Copy link
Contributor

Thanks @Sharcoux we'll take a look at this soon!

@jeffhuang
Copy link
Contributor

I really appreciate you running the lint and investigating these. Most of them seem to be Kalman filter code so I wonder if @xanderkoo would take a look. #4 and #6 are perplexing, and I'll look into them more. #7 is probably old code that was meant for the offline video analysis.

@Sharcoux
Copy link
Author

Hi there. @jeffhuang we came to understand the www part. We're finishing testing all the changes and fixing all potential errors.

@jeffhuang
Copy link
Contributor

Fantastic, thank you so much! If you could scope down the PR to individual fixing, that would be much appreciated. It's harder for us to review lengthier PRs as we'd have to test each fix separately.

@Sharcoux
Copy link
Author

Ok, we finished fixing everything. We had trouble fixing the webpack bundle but solved it easily with vite that is much faster for building anyway and is probably going to be the future of webpack.

About doing individual fixes, it's a bit hard. We didn't implement new features that we could separate from the rest. We just removed duplicated code, add typing and modern code style, and the rest of the evolutions were just consequences of these changes. When the types didn't match, we noticed some errors that we fixed. The original code was also using mjs files, but not importing them as module, but polluting the global environment with a "webgazer" variable instead, which is generally considered a bad practice.

Our next steps will be to simplify the API for users of the lib. I find the current version a bit harsh to use for newcomers.

@Char1x4
Copy link

Char1x4 commented Nov 6, 2024

I am trying to use the webgazer.setStaticVideo api, and encounter the #7 problem. This works in tag 2.0.0, for developers have naming convention problems, and use "videoStream" as both a global variable and the input of function "async function init(videoStream)", that caused the problems in following updates. @jeffhuang you guys may fix this.

@Sharcoux
Copy link
Author

Sharcoux commented Nov 6, 2024

If you want to try, I did a major refacto on my pull request #345

I wanted to go yet a bit further because I think that the data structure for clicks and move is confusing. Having those multiple arrays make it hard to understand the relationship between the data.

I also realized that we could probably train a model to get better results. I was gonna do that but ran out of free time.

@jeffhuang
Copy link
Contributor

@Char1x4 would you mind testing with @Sharcoux 's pull request?

@Char1x4
Copy link

Char1x4 commented Nov 6, 2024

I will tell you after I try, Thank you guys.

@Char1x4
Copy link

Char1x4 commented Nov 10, 2024

Well seems the #7 wrong data type problem not solved. I used a virtual OBS camera to send video stream to normal api instead of using setStaticVideo. Anyway thanks for the response. You may consider write a demo about using "setStaticVideo " later?

@Sharcoux
Copy link
Author

I'm not sure about your exact problem, but the api has changed and you can send the video stream as parameter of the start function. Did that not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants