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 the restriction for minValue and maxValue of MLClampOptions #396

Closed
huningxin opened this issue Jun 9, 2023 · 5 comments · Fixed by #684
Closed

Clarify the restriction for minValue and maxValue of MLClampOptions #396

huningxin opened this issue Jun 9, 2023 · 5 comments · Fixed by #684
Assignees

Comments

@huningxin
Copy link
Contributor

WebNN clamp limits the input tensor element-wise within a range specified by the minimum and maximum values. The minimum and maximum values could be specified by MLClampOptions.

This open item was raised by @quidity (Thanks Alex!) in Chromium CL review which is

can min & max be == floats and is that ok or not?

The current Chromium implementation restricts the min value should be less than or equal to the max value. TensorFlow.js also implements this restriction. However, some backends implement more strict restriction that the min value should be less than max value, like XNNPACK.

WebNN spec should clarify the restriction.

/cc @fdwr, for inputs on DirectML .

@fdwr
Copy link
Collaborator

fdwr commented Jun 17, 2023

WebNN spec should clarify the restriction.

update 2024-04-18 - all known libraries now appear to accept this correctly

tldr: All other tested ML libraries handle empty ranges properly as expected (e.g. min=42, max=42). So my opinion is that we should open an issue to fix the outlier , by removing that suspiciously-probably-a-typo validation check in TF.js. Empty ranges are not an ambiguous corner case:

For inverted ranges though (min > max), library answers differ, and I readily accept either returning an error or just ignoring it like most libraries do. Then for NaN inputs, I'd expect NaN outputs.

Library results for inverted ranges

Given these inverted ranges:

  • input = [0,5,10,15]
  • min = 10
  • max = 5

Yields:

You see 4 different equivalence classes of behavior here:

  • 1️⃣ numpy.clip == tf.clip_by_value == DirectML CLIP
  • 2️⃣ torch.clamp == StableHLO clamp
  • 3️⃣ C++ std::clamp
  • 4️⃣ XNNPack error

NumPy (https://numpy.org/doc/stable/reference/generated/numpy.clip.html)

import numpy

x = numpy.array([0,5,10,15], dtype=numpy.uint8)
y = numpy.clip(x, a_min=10, a_max=10)
print("value:", y)
print("shape:", y.shape)

# Prints:
# value: [10 10 10 10]
# shape: (4,)

TensorFlow (https://www.tensorflow.org/api_docs/python/tf/clip_by_value)

import tensorflow as tf

x = tf.constant([0,5,10,15], dtype=tf.float32)
y = tf.clip_by_value(x, 10, 5); # https://www.tensorflow.org/api_docs/python/tf/clip_by_value
print("value:", y)
print("shape:", y.shape)

# Prints:
# value: tf.Tensor([10. 10. 10. 10.], shape=(4,), dtype=float32)
# shape: (4,)

PyTorch (https://pytorch.org/docs/stable/generated/torch.clamp.html)

import torch

x = torch.tensor([0,5,10,15], dtype=torch.float)
y = torch.clamp(x, min=10, max=5)

print("value:", y)
print("shape:", y.shape)
# Prints:
# value: tensor([5., 5., 5., 5.])
# shape: torch.Size([4])

ONNX (https://github.com/onnx/onnx/blob/main/docs/Operators.md#Clip)

  node {
    input: "input"
    output: "output"
    op_type: "Clip"
    attribute {
      name: "min"
      f: 15
      type: FLOAT
    }
    attribute {
      name: "max"
      f: 10
      type: FLOAT
    }
    domain: ""
  }

DML EP passes. CPU EP fails.

C++ (https://en.cppreference.com/w/cpp/algorithm/clamp)

if x < min, then min
if x > max, then max
else x
#include <iostream>
#include <algorithm>

int main()
{
    int x;
    std::cout << "Given min=10 max=5" << std::endl;
    std::cout << (x = 0 ) << " -> " << std::clamp(x, 10, 5) << std::endl;
    std::cout << (x = 5 ) << " -> " << std::clamp(x, 10, 5) << std::endl;
    std::cout << (x = 10) << " -> " << std::clamp(x, 10, 5) << std::endl;
    std::cout << (x = 15) << " -> " << std::clamp(x, 10, 5) << std::endl;

    return 0;
}

Prints:

Given min=10 max=5
0 -> 10
5 -> 10
10 -> 5
15 -> 5

DirectML (DML_OPERATOR_ELEMENT_WISE_CLIP)

Equivalent to max(minValue, min(x, maxValue)):

dxdispatch.exe models\dml_element_wise_clip.json
Running on 'NVIDIA Quadro P620'
Dispatch 'clip': 1 iterations, 1.0926 ms median (CPU), 0.0051 ms median (GPU)
Resource 'A': 10, 10, 10, 10
{
    "$schema": "./_schema.json",
    
    "resources": 
    {
        "A": 
        {
            "initialValuesDataType": "FLOAT32",
            "initialValues": [0,5,10,15]
        }
    },

    "dispatchables": 
    {
        "clip": 
        {
            "type": "DML_OPERATOR_ELEMENT_WISE_CLIP",
            "desc": 
            {
                "InputTensor": { "DataType": "FLOAT32", "Sizes": [4] },
                "OutputTensor": { "DataType": "FLOAT32", "Sizes": [4] },
                "Min": 10,
                "Max": 5
            }
        }
    },

    "commands": 
    [
        {
            "type": "dispatch",
            "dispatchable": "clip",
            "bindings": 
            {
                "InputTensor": "A",
                "OutputTensor": "A"
            }
        },
        { "type": "print", "resource": "A" }
    ]
}

@wchao1115
Copy link
Collaborator

wchao1115 commented Nov 2, 2023

As a backend API, I think WebNN should not allow a corner case situation that makes it hard for a specific implementation to follow thru e.g. if WebNN supports a degenerate range min == max while XNNPACK can't implement them, that is a burden on the implementer, one that could lead to a corner case latent bug that no one knows about until it hits a customer. Even for the implementation that does support it, it is still likely that they may do so differently from one another depending on their specific policy or other ambiguous circumstances i.e. should the behavior be a no-op, a min filling, or else?

Therefore, in this situation I think it is better to simply assert that min < max, and not min <= max. If the framework on top wants to support a degenerate range, it should be pretty straightforward for them to assert their own policy there independent of WebNN or other backend that they support.

As to whether we should also support inverted range, my rationale is similar to the degenerate range case, it is non-obvious and supporting non-obvious case could lead to a bug-compatibility burden on the implementation side in the future.

@huningxin
Copy link
Contributor Author

XNNPACK allows min and max to be the same: google/XNNPACK@d98c1ff (Thanks @fdwr for the notice)

inexorabletash added a commit to inexorabletash/webnn that referenced this issue Mar 26, 2024
- Documents the convention.

- Dedupes `MLEluOptions`'s `alpha` definitions

- Dedupes `MLClampOptions`'s `minValue` and `maxValue`, and references
  webmachinelearning#396

- Moves `MLOperandDescriptor` member documentation out of IDL comments

- Introduces simple definitions for `MLComputeResult`'s `inputs` and
  `outputs`

- Converts "device type" and "power preference" into definitions for
    `MLContextOptions`'s `deviceType` and `powerPreference`.

- Converts "device type" into MLContextOptions/deviceType definition

Also includes these adjacent changes to improve the document flow:

- Moves the "context type" definition into the MLContext section.

- Moves the Permission Policy Integration section from API into
  Programming Model which seems like a slightly better home for it.

Fixes webmachinelearning#483
inexorabletash added a commit to inexorabletash/webnn that referenced this issue Mar 26, 2024
- Documents the convention.
- Dedupes `MLEluOptions`'s `alpha` definitions
- Dedupes `MLClampOptions`'s `minValue` and `maxValue`, and references
    webmachinelearning#396
- Moves `MLOperandDescriptor` member documentation out of IDL comments
- Introduces simple definitions for `MLComputeResult`'s `inputs` and
    `outputs`
- Converts "device type" and "power preference" into definitions for
    `MLContextOptions`'s `deviceType` and `powerPreference`.

Also includes these adjacent changes to improve the document flow:

- Moves the "context type" definition into the `MLContext` section.
- Moves the Permission Policy Integration section from API into
  Programming Model which seems like a slightly better home for it.

Fixes webmachinelearning#483
fdwr pushed a commit that referenced this issue Mar 28, 2024
* Conventions: Ensure all dict members have definitions

- Documents the convention.
- Dedupes `MLEluOptions`'s `alpha` definitions
- Dedupes `MLClampOptions`'s `minValue` and `maxValue`, and references
    #396
- Moves `MLOperandDescriptor` member documentation out of IDL comments
- Introduces simple definitions for `MLComputeResult`'s `inputs` and
    `outputs`
- Converts "device type" and "power preference" into definitions for
    `MLContextOptions`'s `deviceType` and `powerPreference`.

Also includes these adjacent changes to improve the document flow:

- Moves the "context type" definition into the `MLContext` section.
- Moves the Permission Policy Integration section from API into
  Programming Model which seems like a slightly better home for it.

Fixes #483

* Add subsection for MLContextOptions

* Feedback from @huningxin
@inexorabletash
Copy link
Contributor

tldr: All other tested ML libraries handle empty ranges properly as expected (e.g. min=42, max=42). So my opinion is that we should open an issue to fix the outlier, by removing that suspiciously-probably-a-typo validation check in TF.js. Empty ranges are not an ambiguous corner case:

Hey @fdwr - is there confusion about TF.js here or am I reading your comment wrong? TF's test (util.assert((clipValueMin <= clipValueMax),...) allows empty ranges (min == max). I'm not seeing a glitch in what you linked.

I believe XNNPACK was the only outlier and that's been fixed, so I think we're good to resolve this and remove the note in the spec referencing this issue. Can you confirm, or clarify the remaining concern?

@fdwr
Copy link
Collaborator

fdwr commented Apr 18, 2024

@inexorabletash Indeed, TF.js looks good too. So that's all of them then, and the note should be removed (I can tomorrow). Updated the table above.

inexorabletash added a commit to inexorabletash/webnn that referenced this issue May 9, 2024
Per discussion in the issue, now that XNNPACK is updated there are no
backends that disallow minValue == maxValue, so the issue can be
resolved and spec note can be removed.

Resolves webmachinelearning#396
@inexorabletash inexorabletash self-assigned this May 9, 2024
@fdwr fdwr closed this as completed in #684 May 9, 2024
fdwr pushed a commit that referenced this issue May 9, 2024
…684)

Per discussion in the issue, now that XNNPACK is updated there are no
backends that disallow minValue == maxValue, so the issue can be
resolved and spec note can be removed.

Resolves #396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants