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

(bikeshed naming) MLOperandDataType #454

Closed
fdwr opened this issue Aug 23, 2023 · 6 comments
Closed

(bikeshed naming) MLOperandDataType #454

fdwr opened this issue Aug 23, 2023 · 6 comments

Comments

@fdwr
Copy link
Collaborator

fdwr commented Aug 23, 2023

Various ML libraries accept different kinds of operands to their operators (e.g. TensorFlow might accept tensors or variables; ONNX accepts the operand types dense tensors, sparse tensors, maps, sequences, optionals; ...).

Inside that operand type (e.g. tensor), there may be a subtype for the element data (e.g. float32, bfloat16, int8, int32...), or dtype for short, as found in NumPy data type numpy.dtype, TensorFlow tf.dtype, and PyTorch torch.dtype; or "data type" as found in XNNPack xnn_datatype and DirectML DML_TENSOR_DATA_TYPE.

Sometimes it makes sense in a standard to name something different from general precedent when it is more informative/clearer, but the current naming MLOperandType actually means the data type (float32, int8...), which is both inconsistent with the community (harder to map this term to everything else) and adds no clarity. Additionally, if future WebNN ever supports other kinds of operands besides dense tensors (e.g. sparse tensors), it will be rather awkward to speak of this new type because there will be no vocabulary left for it, since the term MLOperandType was already consumed to mean the subtype within the operand type. Renaming MLOperandType -> MLOperandDataType will enable future expansion (if needed) where MLOperandType can be reintroduced to mean the actual operand type. I liken the current situation to having an enum named Animal which contains dog breeds {Poodle, Bulldog, Boxer}, which makes it harder to speak of actual animal types if later we want to introduce cats.

@zolkis
Copy link
Collaborator

zolkis commented Aug 23, 2023

Valid and well explained. We should have spotted this earlier, in the spirit of https://www.w3.org/TR/design-principles/#naming-is-hard
A quite easy change: should it be part of #446, or do we prefer a separate PR after that one is merged?

@fdwr
Copy link
Collaborator Author

fdwr commented Aug 24, 2023

should it be part of #446, or do we prefer a separate PR after that one is merged?

I defer to you and the editors ✏ there, depending on how much process overhead is involved, but I see that change is already sizeable and appears to be more about style than name changes. There are advantages to smaller changes for history following...

well explained ... naming-is-hard

:) I've been prototyping some extended attributes in SVG for pixel snapping, and yes, naming is a challenge!..

@anssiko
Copy link
Member

anssiko commented Aug 25, 2023

Suggest handle this separately from #446 even if a change in an enum identifier name will not affect web compatibility, is not web-exposed. (A change in an enum value name would be a normative change, needs more care.)

This will be a minor PR on top of the big PR. Thanks for your attention to detail and forward-looking thinking.

@fdwr
Copy link
Collaborator Author

fdwr commented Sep 7, 2023

do we prefer a separate PR after that one is merged?

I see 446 is merged. Shall I create it now?

@zolkis And is there anybody I should tag explicitly for visibility on the code review?

@zolkis
Copy link
Collaborator

zolkis commented Sep 8, 2023

@zolkis I see 446 is merged. Shall I create it now?

Yes, it's a good time for it now.

@inexorabletash
Copy link
Member

Can this be closed out now?

@fdwr fdwr closed this as completed Dec 19, 2023
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

4 participants