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

cleanup conv class definitions #6947

Merged
merged 6 commits into from Oct 20, 2022
Merged

cleanup conv class definitions #6947

merged 6 commits into from Oct 20, 2022

Conversation

vladmandic
Copy link
Contributor

@vladmandic vladmandic commented Oct 15, 2022

with tfjs 4.0 its almost there to a clean build using standard tsc

few minor cleanups:

  • Conv2DTranspose and Conv3DTranspose unnecessarily redeclare inputSpec when its already declared in parent class
  • ConvRNN2D redeclares cell with a different type than parent class, but without explicit declaration

cc @mattsoulanille - can you take a look since you've done most of the work for typescript upgrade?


This change is Reviewable

Copy link
Member

@mattsoulanille mattsoulanille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@@ -706,7 +706,6 @@ serialization.registerClass(Conv3D);
export class Conv2DTranspose extends Conv2D {
/** @nocollapse */
static className = 'Conv2DTranspose';
inputSpec: InputSpec[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Note to other reviewer: This is declared in the Layer interface in src/engine/topology.ts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Matt!

@@ -116,7 +116,7 @@ class ConvRNN2D extends RNN {
/** @nocollapse */
static className = 'ConvRNN2D';

readonly cell: ConvRNN2DCell;
declare readonly cell: ConvRNN2DCell;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch here as well. cell is already a property of RNN, so declare is required here to assert its new type. I'm not completely sure why this didn't throw an error when compiling with Bazel.

Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mattsoulanille mattsoulanille merged commit de68f10 into tensorflow:master Oct 20, 2022
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

Successfully merging this pull request may close these issues.

None yet

3 participants