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.

Sign up for GitHub

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

Jump to bottom

Use both MLOperandDescriptor.dimensions and MLOperandDescriptor.shape #274

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Use both MLOperandDescriptor.dimensions and MLOperandDescriptor.shape #274

merged 3 commits into from
Sep 19, 2024

Conversation

Honry
Copy link
Collaborator

@Honry Honry commented Sep 18, 2024

Fixed #273

@Honry
Copy link
Collaborator Author

Honry commented Sep 18, 2024

@huningxin, @ibelem, PTAL, thanks!

@ibelem
Copy link
Contributor

ibelem commented Sep 18, 2024 •
edited
Loading

Thanks @Honry . What is our plan for such kind of breaking changes @huningxin , I meant do we need to add notes in footer of sample pages? E.g. "Known compatible Chromium version: 130.0.6723.6 due to #676 "

@Honry
Copy link
Collaborator Author

Honry commented Sep 18, 2024

Thanks @Honry . What is our plan for such kind of breaking changes @huningxin , I meant do we need to add notes in footer of sample pages? E.g. "Known compatible Chromium version: 130.0.6723.6 due to #676 "

Maybe we should keep both dimensions and shape for a while to avoid breaking.

@huningxin
Copy link
Contributor

huningxin commented Sep 18, 2024 •
edited
Loading

Maybe we should keep both dimensions and shape for a while to avoid breaking.

sounds good. The Chromium impl would also support both dimensions and shape for a while (a milestone) for a smooth transition.

@Honry
Copy link
Collaborator Author

Honry commented Sep 18, 2024

Maybe we should keep both dimensions and shape for a while to avoid breaking.

sounds good. The Chromium impl would also support both dimensions and shape for a while (a milestone) for a smooth transition.

Done, PTAL, thanks!

ibelem
ibelem approved these changes Sep 18, 2024
View reviewed changes
common/utils.js
@@ -121,7 +121,8 @@ export async function buildConstantByNpy(builder, url, targetType = 'float32') {
throw new Error(`Conversion from ${npArray.dataType} ` +
`to ${targetType} is not supported.`);
}
return builder.constant({dataType: type, dimensions}, typedArray);
return builder.constant(
{dataType: type, dimensions: shape, shape}, typedArray);
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @Honry

@Honry Honry changed the title Rename MLOperandDescriptor.dimensions to MLOperandDescriptor.shape Use both MLOperandDescriptor.dimensions and MLOperandDescriptor.shape Sep 19, 2024
@Honry Honry merged commit 229c4e2 into webmachinelearning:master Sep 19, 2024
3 checks passed
@Honry Honry deleted the rename-dimensions branch September 19, 2024 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibelem ibelem ibelem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename MLOperandDescriptor.dimensions to MLOperandDescriptor.shape
3 participants
@Honry @ibelem @huningxin