Repository navigation
Type of some parameters should match the input data type #442
Description
Activity
Agreed. This may seem like an inconsequential matter because float32 has such a wide range, but float32 cannot accurately represent int32 values over 16 million (all odd numbers get dropped to nearest even), and it will be exacerbated if we extend to larger types like int64. There have already been cases in ONNX models where special sentinel values were used like INT32_MAX which when converted to float32 lose their representation.
🤔 I wonder what the IDL would look like though? (I'm not that familiar with the IDL syntax in its murkier corners)
dictionary MLPadOptions { MLPaddingMode mode = "constant"; ???? value = 0; // What goes here? Is there any sort of multi-valued scalar in JS IDL, like C++ union? }; dictionary MLFillSequenceOptions { ??? start = 0; ??? delta = 1; }; ...Should it use an
orwith multiple types? I see there is precedent for anordescription in split:split(..., (unsigned long or sequence<unsigned long>) splits, ...);Re: "Is there any sort of multi-valued scalar in JS IDL, like C++ union?"
WebIDL has union types
(a or b or c)which can be given a name viatypedef, but the types must be distinguishable. Fundamentally, this part of WebIDL is about mapping incoming JavaScript types into distinct IDL types and applying the appropriate conversion logic. So you can have a union of(DOMString or float)because IDL rules allow distinguishing a JS Number and a JS String into one of those two types. But a union of(float or double)is not permitted, since there's no way to tell which type at the IDL layer to convert a JS Number to.For now, my best suggestion is to accept
unrestricted doublewhich is equivalent to any JS Number (64-bit IEEE 754 FP), and write an algorithm in prose for narrowing, which should reference the same logic as WebIDL e.g. https://webidl.spec.whatwg.org/#es-integer-types and https://webidl.spec.whatwg.org/#es-float etc.In the future, you can do a union of
(bigint and unrestricted double)following the guidance at https://webidl.spec.whatwg.org/#limit-bigint-numeric-unionsOther options include using
anyand write prose for what to accept/reject following the conversions in https://webidl.spec.whatwg.org/#es-any but I think that ends up being a superset of the above.Reacted by Dwayne Robinson, Zoltan Kis and Wanming LinRe:
unrestricted doubleabove - that may have been bad guidance. Are infinities valid? Is NaN valid? If not, just usedouble.Are infinities valid? Is NaN valid?
Josh: Definite yes for infinity. NaN is more arguable, but they have their use (and many libraries have dedicated NaN testing operators -
tf.math.is_nan,torch.isnan, ONNXIsNaN).Reacted by Joshua BellFYI, I have a local change for this, but will wait for the PR queue to drain. I went with this definition:
typedef (bigint or unrestricted double) MLNumber;
And then use MLNumber for constant(value, type), constant(start, end, step, type) (see #571 and #492), MLClampOptions and MLPadOptions.
unrestricted- because Infinity should be allowed, per abovedouble- per Clarify the usage of 32 bit floating point type and consider using double #325 it's unclear that limiting to float in the IDL is useful unless we really really want the semantics of https://webidl.spec.whatwg.org/#js-float
Bikeshedding on the name is welcome. :)
Reacted by Dwayne Robinson and Ningxin HuFYI, I have a local change for this, but will wait for the PR queue to drain. I went with this definition:
typedef (bigint or unrestricted double) MLNumber;
...
Bikeshedding on the name is welcome. :)@inexorabletash Yeah, I suppose it makes sense to just have a common numeric scalar type (rather than repeated definitions in each of the function prototypes) if we're going to be sharing this across multiple operators - clamp, pad, fill constant, fill sequence, diagonal matrix... I originally thought including "scalar" in the name would make sense, but then JS has a "Number" type, and so
MLNumbermakes sense. So, 👍.typedef (bigint or unrestricted double) MLNumber;
looks good!
- added a commit that references this issue
on Jul 7, 2024
pad(),
MLPadOptions::valueis float in current spec, which is used as padded value if theMLPadOptions::modeis "constant", its type should exactly match the input data type, otherwise this may cause precision loss.(ONNX Pad-18 requires the input data and constant value (a single scalar input tensor) to be the same type T.)
clamp(),
MLClampOptions::minValueandMLClampOptions::maxValue, current spec states them as a float scalar.And for the v2 ops, we will also have to consider these: (thanks @fdwr for providing the list)