-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add linting rules for tfjs-models. #333
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad this is finally happening!
Another PR in the future could run linting on the demos as part of CI.
Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)
body-pix/src/base_model.ts, line 32 at r1 (raw file):
* added to BodyPix. */ export abstract class BaseModel {
Awesome refactoring!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really really nice cleanup!!
Reviewed 24 of 36 files at r1, 55 of 56 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
body-pix/src/mobilenet.ts, line 38 at r2 (raw file):
// tslint:disable-next-line:no-any export function assertValidResolution(resolution: any, outputStride: number) {
looks like a lot of code went away (didn't move). Were these unused methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
body-pix/src/base_model.ts, line 64 at r2 (raw file):
* - partHeatmaps: A Tensor3D that represents the body part segmentation. */ predict(input: tf.Tensor3D): {
Do we need to specify return type here?
body-pix/src/base_model.ts, line 96 at r2 (raw file):
// Because MobileNet and ResNet predict() methods output a different order for // these values, we have a method that needs to be implemented to order them. abstract nameOutputResults(results: tf.Tensor3D[]): {
Do we need to specify return type here?
body-pix/src/mobilenet.ts, line 38 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
looks like a lot of code went away (didn't move). Were these unused methods?
It's good. All the validation is now performed at the body_pix_model.ts level (1 level higher).
Thx LGTM for the changes under body-pix/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tylerzhu-github)
body-pix/src/base_model.ts, line 64 at r2 (raw file):
Previously, tylerzhu-github (tylerzhu-github) wrote…
Do we need to specify return type here?
In general it's better to have a stricter return type. This allows people who call predict() to know what comes out (and also prevents against string typos).
body-pix/src/base_model.ts, line 96 at r2 (raw file):
Previously, tylerzhu-github (tylerzhu-github) wrote…
Do we need to specify return type here?
See above.
body-pix/src/mobilenet.ts, line 38 at r2 (raw file):
Previously, tylerzhu-github (tylerzhu-github) wrote…
It's good. All the validation is now performed at the body_pix_model.ts level (1 level higher).
Ack.
This change is