Add the clamp() algorithm #348

zolkis · 2023-02-15T19:06:04Z

Add the clamp algorithm. Tracked in #347 (discussion there).
Depends on #336.

zolkis · 2023-02-16T16:45:46Z

During the call today, @huningxin you made a remark that an MLActivation is not needed when an MLOperator is returned. To me it seems it was needed in the impl, also for the step of connecting the op input/output. But welcome to suggest better steps here, thanks!

huningxin · 2023-02-16T23:43:49Z

@zolkis

To me it seems it was needed in the impl, also for the step of connecting the op input/output. But welcome to suggest better steps here, thanks!

The impl you referred to returns an MLOperand.

This impl returns an MLOperator (and should be updated to MLActivation)

zolkis · 2023-02-23T19:56:28Z

@zolkis

To me it seems it was needed in the impl, also for the step of connecting the op input/output. But welcome to suggest better steps here, thanks!

The impl you referred to returns an MLOperand.

This impl returns an MLOperator (and should be updated to MLActivation)

We have a single algorithm for clamp() in JS, and we distinguish for the return values based on the arguments provided to the function. So the two impl need to be combined there. Of course in cpp the implementation can be polymorphic. AFAIK we cannot have 2 separate algorithms mirroring a cpp implementation. We need to figure out what we want in JS, and then implement in cpp. As this is the same case with almost every other function in the API, we need to get this right.
I added a note to clarify the algorithm.

zolkis · 2023-02-23T20:12:25Z

Also, I will need to update #337 with internal constructors for MLOperand and MLActivation as needed in the steps of the clamp() algorithm. I think it's better to add those commits to that PR, not this one.

zolkis · 2023-03-18T14:05:58Z

I modified the algorithm like they would be when #337 is merged. This shows how the create MLOperand and create MLActivation steps would be used.

zolkis · 2023-03-21T22:29:35Z

I am preparing the PR for input() and constant(), and that changes this PR, too. Moving it back to WiP.

zolkis · 2023-03-22T12:26:46Z

index.bs

+    The {{MLGraphBuilder/clamp()}} method steps are:
+    1. Let |arg1| be the first argument.
+    1. If |arg1| is an instance of {{MLOperand}}, run the {{MLGraphBuilder/clamp(operand, options)}} steps with all the received arguments.
+    1. Otherwise, run the {{MLGraphBuilder/clamp(options)}} steps with all received arguments.


@huningxin : in general, for polymorphic functions, do we prefer adding these boilerplate lines, or just define the two forms separately?

wchao1115 · 2023-04-12T22:07:46Z

index.bs

+        <div class="note">
+            This step does not execute the clamp operation yet, it just prepares the operator.
+        </div>


I don't think this note is needed because the same statement can also be made for all the other graph builder methods that add a given operation into the graph i.e. nothing is executed yet.

Indeed it would be better to state that once for all the ops and remove the repetitive notes.

zolkis · 2023-04-20T13:33:39Z

@wchao1115 concern addressed, PTAL.

zolkis · 2023-05-16T15:49:44Z

@huningxin @wchao1115 PTAL: squashed to one commit.

Squashed from the following commits: Replace MLOperand.[[descriptor]] with type and dimensions Clarify the algorithm for only setting up the op Improve the clamp() algorithm, use the prose assuming the create steps for MLOperand and MLActivation Rework clamp with polymorphic behavior. Update for changes in MLOperand. Rework clamp() like constant(), polymorphic forms in separate sections, argument and return descriptions as notes. Fix platform related steps and reference to internal slots Address review, remove note Remove back quotes from title Signed-off-by: Zoltan Kis <[email protected]>

Signed-off-by: Zoltan Kis <[email protected]>

huningxin · 2023-06-01T13:43:38Z

index.bs

+    1. Set |activation|.{{MLActivation/[[builder]]}} to |builder|.
+    1. Set |activation|.{{MLActivation/[[name]]}} to |name|.
+    1. If |options| is an [=object=], set |activation|.{{MLActivation/[[options]]}} to |options|.
+    1. Make a request to the underlying platform to bind the [=implementation-defined=] platform operator for |name| to |activation|.{{MLActivation/[[operator]]}}.


Should we leave the platform operator creation to each build method? Because I suppose the creation steps would be different, e.g. handling of options.

I agree. Let's start with that assumption. We can always cleanly factor out common things when the opportunity comes.

huningxin · 2023-06-01T13:46:45Z

index.bs

+    1. If |operand| is not an instance of {{MLOperand}}, then throw a "{{TypeError}}" and stop.
+    1. Let |result| be a new [=object=].
+    1. Set |result|.{{MLOperand/[[builder]]}} to |operand|.{{MLOperand/[[builder]]}}.
+    1. Set |result|.{{MLOperand/[[descriptor]]}} to |operand|.{{MLOperand/[[descriptor]]}}.


Should we do deep copy of the descriptor? Would the change of original operand's descriptor impact the copied one?

All internal slots should be immutable, i.e. only changeable with API calls, so assignments to internal slots should involve deep copies by default. I will check this.

ECMAScript doesn't really shed light on this. Internal slots are supposed to be created and then not modified unless specified differently.

Now we seem to have 2 options:

change every line that sets an internal slot to replace the argument with "a copy of the argument"

or, create steps to "set the internal slot [[name]] to <arg>" steps, where we say "set [[name]] to a copy of <arg>".

@anssiko @dontcallmedom any input here?

@huningxin
Since this is a general issue for the spec, could we create an issue and fix it outside this PR, in all affected places?

I am fine to fix it in a separate issue. Please create that issue and add link into the spec. Thanks.

huningxin · 2023-06-01T13:54:29Z

index.bs

+        1. If running the <a>check clamp options</a> steps with |options| returns `false`, then throw a "{{TypeError}}" {{DOMException}} and abort these steps.
+    1. Let |result| be the result of invoking the <a>copy MLOperand</a> steps given |operand|.
+            1. If that throws an error, re-throw the error and abort these steps.
+    1. Make a request to the underlying platform to connect |result| with the [=implementation-defined=] platform operator for clamp, and store a reference to the resulting [=implementation-defined=] platform operand object in |result|.{{MLOperand/[[operand]]}}.


I suppose there are a few sub-steps:

Make a request to the underlying platform to create an [=implementation-defined=] platform operator for clamp with |options|.{{MLClampOptions/minValue}} and |options|.{{MLClampOptions/minValue}}.

Register the |operand|.{{MLOperand/[[operand]]}} as input to the platform operator for clamp.

Register the |result|.{{MLOperand/[[operand]]}} as output to the platform operator for clamp.

Store a reference to the platform operator for clamp in |result|.{{MLOperand/[[operator]]}}.

index.bs

huningxin · 2023-06-01T14:18:51Z

index.bs

+        1. If running the <a>check clamp options</a> steps with |options| returns `false`, then throw a "{{TypeError}}" {{DOMException}} and abort these steps.
+    1. Let |op| be the result of invoking the <a>create MLActivation</a> steps with `"clamp"` and |options|.
+        1. If that throws an error, re-throw the error and abort these steps.
+    1. Make a request to the underlying platform to connect |op| with the [=implementation-defined=] platform operator for clamp, and store a reference to the resulting [=implementation-defined=] platform operator object in |op|.{{MLActivation/[[operator]]}}.


I suppose we can leverage the steps of {{MLGraphBuilder/clamp(operand, options)}} method. The difference is here it doesn't need register input and output operand.

This is for activation and let's try its own steps. Let's keep track if we could reuse these operand and activation platform-related steps in other methods and how to parametrize them. If there is an opportunity, we can factor them out later.

huningxin · 2023-06-01T14:26:34Z

index.bs

+    1. Let |result| be a new [=object=].
+    1. Set |result|.{{MLOperand/[[builder]]}} to |operand|.{{MLOperand/[[builder]]}}.
+    1. Set |result|.{{MLOperand/[[descriptor]]}} to |operand|.{{MLOperand/[[descriptor]]}}.
+    1. Set |result|.{{MLOperand/[[name]]}} to |operand|.{{MLOperand/[[name]]}}.


{{MLOperand/[[name]]}} is only valid for input operand, it should not be copied if the destination operand is not an input.

huningxin · 2023-06-01T14:28:58Z

index.bs

+        - an {{MLOperand}}. The output tensor of the same shape as *operand*.
+</div>
+<div algorithm=clamp>
+    The {{MLGraphBuilder/clamp(operand, options)}} method steps are:


operand is commonly used, e.g. for MLOperand or platform operand etc. Should the variable name be more specific, like input or x?

Oh, so that was the reason for using x. Well, input is also a loaded name :).
Since it is an MLOperand, I think using operand is just fine.

Signed-off-by: Zoltan Kis <[email protected]>

huningxin

LGTM with a follow-up for "deep-copy" issue.

huningxin · 2023-06-05T12:00:15Z

index.bs

+    1. If |operand| is not an instance of {{MLOperand}}, then throw a "{{TypeError}}" and stop.
+    1. Let |result| be a new [=object=].
+    1. Set |result|.{{MLOperand/[[builder]]}} to |operand|.{{MLOperand/[[builder]]}}.
+    1. Set |result|.{{MLOperand/[[descriptor]]}} to |operand|.{{MLOperand/[[descriptor]]}}.


I am fine to fix it in a separate issue. Please create that issue and add link into the spec. Thanks.

zolkis · 2023-06-13T15:09:30Z

Moved to #399 with new target branch.

anssiko mentioned this pull request Feb 16, 2023

Use modern WebIDL and Infra standard conventions #210

Closed

zolkis changed the title ~~[WiP] Add the clamp() algorithm~~ Add the clamp() algorithm Mar 19, 2023

zolkis changed the title ~~Add the clamp() algorithm~~ WiP: Add the clamp() algorithm Mar 21, 2023

zolkis commented Mar 22, 2023

View reviewed changes

zolkis force-pushed the clamp-algorithm branch from a6522ec to f8872c7 Compare March 22, 2023 12:30

zolkis mentioned this pull request Mar 22, 2023

Add the constant() method steps. #365

Merged

zolkis force-pushed the clamp-algorithm branch from 58d2460 to cc08d5d Compare March 22, 2023 16:40

wchao1115 requested changes Apr 12, 2023

View reviewed changes

zolkis changed the title ~~WiP: Add the clamp() algorithm~~ Add the clamp() algorithm Apr 13, 2023

zolkis force-pushed the clamp-algorithm branch from af38f15 to 3f81f3a Compare May 16, 2023 15:48

anssiko requested a review from huningxin May 24, 2023 06:28

zolkis added 2 commits May 25, 2023 19:03

Add the 'copy MLOperand' and 'create MLActivation' steps

d68bd87

Signed-off-by: Zoltan Kis <[email protected]>

zolkis force-pushed the clamp-algorithm branch from 3f81f3a to d68bd87 Compare May 25, 2023 16:13

huningxin reviewed Jun 1, 2023

View reviewed changes

clamp(): improve platform related steps

dc4d3d6

Signed-off-by: Zoltan Kis <[email protected]>

zolkis requested a review from huningxin June 2, 2023 14:27

huningxin approved these changes Jun 5, 2023

View reviewed changes

zolkis mentioned this pull request Jun 5, 2023

Use deep copy for setting internal slots? #395

Closed

zolkis mentioned this pull request Jun 13, 2023

Clamp algorithm #399

Merged

zolkis closed this Jun 13, 2023

This was referenced Jun 20, 2023

Staging area: discuss issues for the zk-conventions-integration branch #404

Closed

Add internal slots to MLOperand, MLActivation and basic algorithms #336

Closed

zolkis deleted the clamp-algorithm branch November 12, 2024 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the clamp() algorithm #348

Add the clamp() algorithm #348

zolkis commented Feb 15, 2023 •

edited by pr-preview bot

Loading

zolkis commented Feb 16, 2023

huningxin commented Feb 16, 2023

zolkis commented Feb 23, 2023

zolkis commented Feb 23, 2023

zolkis commented Mar 18, 2023

zolkis commented Mar 21, 2023

zolkis Mar 22, 2023

wchao1115 Apr 12, 2023

zolkis Apr 13, 2023

zolkis Apr 13, 2023

zolkis commented Apr 20, 2023

zolkis commented May 16, 2023

huningxin Jun 1, 2023

zolkis Jun 1, 2023 •

edited

Loading

huningxin Jun 1, 2023

zolkis Jun 1, 2023

zolkis Jun 2, 2023 •

edited

Loading

zolkis Jun 2, 2023

huningxin Jun 5, 2023

huningxin Jun 1, 2023

huningxin Jun 1, 2023

zolkis Jun 2, 2023

huningxin Jun 1, 2023

huningxin Jun 1, 2023

zolkis Jun 2, 2023

huningxin left a comment

huningxin Jun 5, 2023

zolkis commented Jun 13, 2023

Add the clamp() algorithm #348

Add the clamp() algorithm #348

Conversation

zolkis commented Feb 15, 2023 • edited by pr-preview bot Loading

zolkis commented Feb 16, 2023

huningxin commented Feb 16, 2023

zolkis commented Feb 23, 2023

zolkis commented Feb 23, 2023

zolkis commented Mar 18, 2023

zolkis commented Mar 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolkis commented Apr 20, 2023

zolkis commented May 16, 2023

Choose a reason for hiding this comment

zolkis Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolkis Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huningxin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolkis commented Jun 13, 2023

zolkis commented Feb 15, 2023 •

edited by pr-preview bot

Loading

zolkis Jun 1, 2023 •

edited

Loading

zolkis Jun 2, 2023 •

edited

Loading