Skip to content

Allow NaN and infinite values for float attributes #800

Description

@lisa0314

This issue was raised in this CL review- https://chromium-review.googlesource.com/c/chromium/src/+/6081404/5..10/third_party/blink/web_tests/external/wpt/webnn/validation_tests/elu.https.any.js#b50.

The current blink IDL uses float alpha while spec defines as double alpha. I suppose both have issues and it should be defined as unrestricted double which supports both infinite and NaN.

Activity

  1. inexorabletash commented on Dec 16, 2024

    @inexorabletash
    Contributor

    See here and here for past discussion. We settled on not adding unrestricted since we didn't think there were use cases for allowing this class inputs to include infinities and NaNs, and that early error for developers was better.

    https://g-issues.chromium.org/issues/382108345 doesn't provide use cases either, it's just pointing out where the impl doesn't match the spec.

    Are there use cases, e.g. existing models where ORT Web would generate e.g. elu() calls with Infinities or NaNs? If so, happy to revisit!

  2. fdwr commented on Dec 25, 2024

    @fdwr
    Collaborator

    Use-case-wise, I know no benefit 🤔 (and for elu in particular, I expect no inf/nan), leaving as a remaining argument broadly applied principled stances, as it is reasonable for self-consistency. Note also the parameter being passed by option-dict value rather than inside a tensor is really the only reason this is even in discussion, because if it was inside the tensor, it would just work as expected per normal IEEE rules.

    • ➖ No known use case for the operator - the output wouldn't be meaningful for machine learning.
    • ➖ Not observed in known real world models AFAIK.
    • ➖ Rejecting early could catch errors faster. Granted, I only weakly follow this argument since we expect these parameters to be ferried along from some existing source like an already published model, rather than directly programmatically called, meaning the value would already be in the model, and rejecting it would only hurt compat rather than help diagnose a false positive.
    • ➖ IIRC some backends error on inf/nan for elu's "alpha" (expScale) value, meaning that supporting inf/nan would require explicit decomposition.
    • ➕ Self-consistency with the operator's equivalent decomposition to more primitive operators, where inf/nan behave per IEEE.
    • ➕ Self-consistency with passing the value via scalar tensor instead, where inf/nan would just work (since passing it via tensor affords no prevalidation on the GPU).
    • ➕ Self-consistency with certain other operators which accept non-tensor values, like clamp which supports non-finite values. Though, arguably these operators were promoted to MLNumber (typedef (bigint or unrestricted double)) for this reason and do not remain double anymore.

    Looking at the current operators that still use double parameters (rather than a tensor or MLNumber), we see...

    dictionary MLBatchNormalizationOptions : MLOperatorOptions {
      ...
      double epsilon = 1e-5;
    };
    
    dictionary MLEluOptions : MLOperatorOptions {
      double alpha = 1;
    };
    
    dictionary MLGemmOptions : MLOperatorOptions {
      ...
      double alpha = 1.0;
      double beta = 1.0;
      ...
    };
    
    dictionary MLHardSigmoidOptions : MLOperatorOptions {
      double alpha = 0.2;
      double beta = 0.5;
    };
    
    dictionary MLInstanceNormalizationOptions : MLOperatorOptions {
      ...
      double epsilon = 1e-5;
      ...
    };
    
    dictionary MLLayerNormalizationOptions : MLOperatorOptions {
      ...
      double epsilon = 1e-5;
    };
    
    dictionary MLLeakyReluOptions : MLOperatorOptions {
      double alpha = 0.01;
    };
    
    dictionary MLLinearOptions : MLOperatorOptions {
      double alpha = 1;
      double beta = 0;
    };

    None of these have a strong use case to support inf/nan AFAICS, and the pro's and con's don't show a clear winner above for making the parameters unrestricted. So my view remains neutral and to keep the status quo, but if a use case is shown, or if a model is found that uses the values for some reason, I'd argue to support unrestricted.

  3. huningxin commented on Dec 30, 2024

    @huningxin
    Contributor

    +1 to keep the status quo.

  4. huningxin commented on Jan 16, 2025

    @huningxin
    Contributor

    Implementation (Chromium/Blink) issue created: https://issues.chromium.org/issues/386611711, closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions