Fix various issues with murmurhash. (#514)

* Fix murmurhash when multiple strings are in the input. Also add an attribute to specify output type like sckitlearn. Add type inference to decide output type based on this attribute.

* Address PR comment

onnxruntime/contrib_ops/cpu/murmur_hash3.cc

@@ -3,11 +3,10 @@
 // domain. The author hereby disclaims copyright to this source code.
 
 //scikit-learn is a Python module for machine learning built on top of SciPy and
-//distributed under the 3-Clause BSD license. See https://github.com/scikit-learn/scikit-learn. 
+//distributed under the 3-Clause BSD license. See https://github.com/scikit-learn/scikit-learn.
 //This material is licensed under the BSD License (see https://github.com/scikit-learn/scikit-learn/blob/master/COPYING);
 /* Modifications Copyright (c) Microsoft. */
 
-
 #include "contrib_ops/cpu/murmur_hash3.h"
 
 // Platform-specific functions and macros
@@ -157,7 +156,11 @@ void MurmurHash3::MurmurHash3_x86_32(const void* key, int len, uint32_t seed, vo
 
   h1 = fmix(h1);
 
-  *(uint32_t*)out = h1;
+  if (is_positive_) {
+    *(uint32_t*)out = h1;
+  } else {
+    *(int32_t*)out = h1;
+  }
 }
 
 Status MurmurHash3::Compute(OpKernelContext* ctx) const {
@@ -179,7 +182,7 @@ Status MurmurHash3::Compute(OpKernelContext* ctx) const {
       MurmurHash3_x86_32(input_string.c_str(),
                          static_cast<int>(input_string.length()),
                          seed_,
-                         reinterpret_cast<uint32_t*>(output) + static_cast<int64_t>(i) * output_element_bytes);
+                         reinterpret_cast<uint8_t*>(output) + static_cast<int64_t>(i) * output_element_bytes);
     } else {
       auto output_type = output_tensor->DataType();
       if ((DataTypeImpl::GetType<int32_t>() == keys_type || DataTypeImpl::GetType<uint32_t>() == keys_type) &&

onnxruntime/contrib_ops/cpu/murmur_hash3.h

@@ -13,6 +13,7 @@ class MurmurHash3 final : public OpKernel {
  public:
   MurmurHash3(const OpKernelInfo& info) : OpKernel(info) {
     seed_ = static_cast<uint32_t>(info.GetAttrOrDefault<int64_t>("seed", 0));
+    is_positive_ = static_cast<int64_t>(info.GetAttrOrDefault<int64_t>("positive", 1));
   }
 
   Status Compute(OpKernelContext* context) const override;
@@ -22,6 +23,7 @@ private:
 
 private :
   uint32_t seed_;
+  int64_t is_positive_{1};
 };
 }  // namespace contrib
 }  // namespace onnxruntime

onnxruntime/core/graph/contrib_ops/contrib_defs.cc

@@ -340,7 +340,7 @@ activation.)DOC")
       .SetDomain(kMSDomain)
       .SinceVersion(1)
       .SetDoc(R"DOC(
-The FusedGemm operator schema is the same as Gemm besides it includes attributes 
+The FusedGemm operator schema is the same as Gemm besides it includes attributes
 activation and leaky_relu_alpha.)DOC")
       .Input(
           0,
@@ -477,32 +477,32 @@ activation and leaky_relu_alpha.)DOC")
   If the maximum number of tokens found per input string is D, the output shape would be [N, C, D] when input shape is [N, C].
   Similarly, if input shape is [C] then the output should be [C, D]. Tokenizer has two different operation modes.
   The first mode is selected when "tokenexp" is not set and "separators" is set. If "tokenexp" is set and "separators" is not set,
-  the second mode will be used. The first mode breaks each input string into tokens by removing separators. 
+  the second mode will be used. The first mode breaks each input string into tokens by removing separators.
 
-  Let's assume "separators" is [" "] and consider an example. 
-  If input is 
+  Let's assume "separators" is [" "] and consider an example.
+  If input is
 
-  ["Hello World", "I love computer science !"] whose shape is [2], 
+  ["Hello World", "I love computer science !"] whose shape is [2],
 
-  then the output would be 
+  then the output would be
 
- [["Hello", "World", padvalue, padvalue, padvalue], 
- ["I", "love", "computer", "science", "!"]] 
+ [["Hello", "World", padvalue, padvalue, padvalue],
+ ["I", "love", "computer", "science", "!"]]
 
- whose shape is [2, 5] because you can find at most 5 tokens per input string. 
+ whose shape is [2, 5] because you can find at most 5 tokens per input string.
  Note that the input at most can have two axes, so 3-D and higher dimension are not supported.
 
  For each input string, the second mode searches matches of "tokenexp" and each match will be a token in Y.
  The matching of "tokenexp" is conducted greedily (i.e., a match should be as long as possible).
- This operator searches for the first match starting from the beginning of the considered string, 
+ This operator searches for the first match starting from the beginning of the considered string,
  and then launches another search starting from the first remained character after the first matched token.
  If no match found, this operator will remove the first character from the remained string and do another search.
  This procedure will be repeated until reaching the end of the considered string.
 
-  Let's consider another example to illustrate the effect of setting "mark" to true. 
-  If input is ["Hello", "World"], 
+  Let's consider another example to illustrate the effect of setting "mark" to true.
+  If input is ["Hello", "World"],
   then the corresponding output would be [0x02, "Hello", "World", 0x03].
-  This implies that if mark is true, [C]/[N, C] - input's output shape becomes [C, D+2]/[N, C, D+2]. 
+  This implies that if mark is true, [C]/[N, C] - input's output shape becomes [C, D+2]/[N, C, D+2].
 )DOC";
 
   ONNX_CONTRIB_OPERATOR_SCHEMA(Tokenizer)
@@ -1058,13 +1058,29 @@ The bounding box coordinates corresponding to the selected indices can then be o
       .Input(0, "X", "An input tensor to hash.", "T1")
       .Output(0, "Y", "32-bit hash value.", "T2")
       .TypeConstraint("T1", {"tensor(uint32)", "tensor(int32)", "tensor(string)"}, "Constrain input type to unsigned or signed 32-bit integer tensor, or string tensor. It should be utf-8 encoded if using unicode.")
-      .TypeConstraint("T2", {"tensor(uint32)", "tensor(int32)"}, "Constrain output type to unsigned or signed 32-bit integer tensor.")
+      .TypeConstraint("T2", {"tensor(uint32)", "tensor(int32)"}, "Constrain output type to unsigned and signed 32-bit integer tensor.")
       .Attr(
           "seed",
           "Seed for the hashing algorithm, unsigned 32-bit integer, default to 0.",
           AttributeProto::INT,
           (int64_t)0LL)
+      .Attr(
+          "positive",
+          "If value is 1, output type is uint32_t, else int32_t. Default value is 1.",
+          AttributeProto::INT,
+          (int64_t)1LL)
       .TypeAndShapeInferenceFunction([](ONNX_NAMESPACE::InferenceContext& ctx) {
+        // type inference
+        auto positive_attr = ctx.getAttribute("positive");
+        bool is_positive =
+            positive_attr ? (static_cast<int>(positive_attr->i()) == 1 ? true : false) : true /* default value if attribute not present */;
+        auto output_data_type = ctx.getOutputType(0)->mutable_tensor_type();
+        if (is_positive) {
+          output_data_type->set_elem_type(::onnx::TensorProto_DataType::TensorProto_DataType_UINT32);
+        } else {
+          output_data_type->set_elem_type(::onnx::TensorProto_DataType::TensorProto_DataType_INT32);
+        }
+
         // Shape inference
         if (!hasInputShape(ctx, 0))
           return;

onnxruntime/test/contrib_ops/murmur_hash3_test.cc

@@ -10,6 +10,7 @@ namespace test {
 TEST(MurmurHash3OpTest, DefaultSeed) {
   OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
   test.AddInput<int32_t>("X", {1}, {3L});
+  test.AddAttribute<int64_t>("positive", 0);
   test.AddOutput<int32_t>("Y", {1}, {847579505L});
   test.Run();
 }
@@ -18,6 +19,7 @@ TEST(MurmurHash3OpTest, ZeroSeed) {
   OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
   test.AddInput<int32_t>("X", {1}, {3L});
   test.AddAttribute<int64_t>("seed", 0LL);
+  test.AddAttribute<int64_t>("positive", 0);
   test.AddOutput<int32_t>("Y", {1}, {847579505L});
   test.Run();
 }
@@ -46,10 +48,11 @@ TEST(MurmurHash3OpTest, MoreData) {
   test.Run();
 }
 
-TEST(MurmurHash3OpTest,NonZeroSeed) {
+TEST(MurmurHash3OpTest, NonZeroSeed) {
   OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
   test.AddInput<int32_t>("X", {1}, {3L});
   test.AddAttribute<int64_t>("seed", 42LL);
+  test.AddAttribute<int64_t>("positive", 0);
   test.AddOutput<int32_t>("Y", {1}, {-1823081949L});
   test.Run();
 }
@@ -66,6 +69,7 @@ TEST(MurmurHash3OpTest, StringKeyIntResult) {
   OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
   test.AddInput<std::string>("X", {1}, {"foo"});
   test.AddAttribute<int64_t>("seed", 0LL);
+  test.AddAttribute<int64_t>("positive", 0);
   test.AddOutput<int32_t>("Y", {1}, {-156908512L});
   test.Run();
 }
@@ -78,10 +82,19 @@ TEST(MurmurHash3OpTest, StringKeyUIntResult) {
   test.Run();
 }
 
+TEST(MurmurHash3OpTest, MultipleStringsKeyUIntResult) {
+  OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
+  test.AddInput<std::string>("X", {2}, {"foo", "bar"});
+  test.AddAttribute<int64_t>("seed", 0LL);
+  test.AddOutput<uint32_t>("Y", {2}, {4138058784L, 1158584717L});
+  test.Run();
+}
+
 TEST(MurmurHash3OpTest, StringKeyIntWithSeed42) {
   OpTester test("MurmurHash3", 1, onnxruntime::kMSDomain);
   test.AddInput<std::string>("X", {1}, {"foo"});
   test.AddAttribute<int64_t>("seed", 42LL);
+  test.AddAttribute<int64_t>("positive", 0);
   test.AddOutput<int32_t>("Y", {1}, {-1322301282L});
   test.Run();
 }