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

Allow NaN and infinite values for float attributes #800

Closed
lisa0314 opened this issue Dec 16, 2024 · 4 comments
Closed

Allow NaN and infinite values for float attributes #800

lisa0314 opened this issue Dec 16, 2024 · 4 comments

Comments

@lisa0314
Copy link

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.

@inexorabletash
Copy link
Member

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!

@fdwr
Copy link
Collaborator

fdwr commented Dec 25, 2024

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.

@huningxin
Copy link
Contributor

huningxin commented Dec 30, 2024

+1 to keep the status quo.

@huningxin
Copy link
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
Projects
None yet
Development

No branches or pull requests

5 participants