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

Clarify intermediate values (activations) vs MLActivation vs operations #335

Closed
zolkis opened this issue Feb 1, 2023 · 5 comments
Closed

Comments

@zolkis
Copy link
Collaborator

zolkis commented Feb 1, 2023

With consecutive recent changes I feel we should check and sort out the use of the following terms in the spec:

  • intermediate values (activations)
  • MLActivation interface (former MLOperator)
  • operations (both ops and activation functions).

The latter are supposed to have functional semantics, so don't have state and don't change states of other objects. Therefore they are fine being represented by ops functions in MLGraphBuilder.

However,

  1. Should we consider marking the activation functions among ops?
  2. Would a separate MLOperator definition be still useful, as it is continued to be used in implementation(s)?

(Possibly a V2 discussion, but would be nice to sort out in V1. The text definitely needs checking in V1.)
If there is a clear explanation, we just need to update the spec text, explainer etc.

@zolkis
Copy link
Collaborator Author

zolkis commented Mar 1, 2023

Having worked on several algorithms and having studied the implementation, I don't find the current usage of MLActivation sufficiently clear.

I suggest we revert to the MLOperator interface name, and use the activation name in arguments with the type MLOperator, for instance in batchNormalization(), conv2d(), etc.

In addition, we need constructors for MLOperator and MLOperand (I have written them, but didn't make a PR).

This would help make the spec more clear and also follow at least the CPU impl more closely.

@huningxin
Copy link
Contributor

@zolkis , thanks for your feedback.

There is a review-in-progress Chromium CL that exposes MLActivation and makes MLOperator internal. You may want to check it out. Thanks!

@zolkis
Copy link
Collaborator Author

zolkis commented Mar 2, 2023

Thanks @huningxin.
We still have a problem in the spec that MLGraph is under-specified and MLActivation is an empty interface.
I have made an attempt to address the latter, and it only requires an internal slot for a name, and eventually to an internal function.
We should also think about expressing the graph structure of MLGraph more exactly, so that we could refer to it in algorithms with more formalized prose than English. :)

@inexorabletash
Copy link
Member

@zolkis - do you think we need to keep this open, or is it covered by more specific issues e.g. #448 #457 #549 #552 ? (Many of which are overlapping/redundant too...)

@zolkis
Copy link
Collaborator Author

zolkis commented Feb 22, 2024

This is an old problem/discussion that has been addressed from multiple sides during the past year. Closing this for now.

@zolkis zolkis closed this as completed Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants