You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/08/03 05:53:37 UTC
[GitHub] [tvm] jwfromm opened a new pull request, #12284: [Relay][Op] Multinomial
jwfromm opened a new pull request, #12284:
URL: https://github.com/apache/tvm/pull/12284
This PR introduces the `multinomial` random operator. It's a neat adaptation of `random.uniform` that allows weighted selection of indices from a probability tensor. This op is used in new Dalle-like architectures to generate random images. The PR provides a topi implemenation and tests, relay integration, and an initial pytorch integration. I did not implement sampling without replacement at this time as it seems complicated to do as a tensor operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936900244
##########
python/tvm/relay/frontend/pytorch.py:
##########
@@ -3386,6 +3386,19 @@ def trilu(self, inputs, input_types, mode):
upper = True if mode == "triu" else False
return _op.trilu(data, k, upper)
+ def multinomial(self, inputs, input_types):
+ probs = inputs[0]
+ num_samples = inputs[1]
+ replacement = inputs[2] if inputs[2] else True
+ assert not (
+ replacement is False and num_samples > 1
+ ), "Multinomial without replacement is not yet supported."
+ seed = np.random.randint(1e6)
Review Comment:
Yeah thats a good point, we'd have to use a global dictionary or something for that. I'll add a note. For now, this approach matches how we handle other rng importer functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on PR #12284:
URL: https://github.com/apache/tvm/pull/12284#issuecomment-1203514514
@sfvaroglu can you take a look at this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r937999100
##########
python/tvm/topi/random/kernel.py:
##########
@@ -602,3 +602,57 @@ def normal(gen, mean, scale, out_shape, out_dtype):
)
return new_gen, random_values
+
+
+def multinomial(gen, probs, num_samples):
+ """Draw samples from a multinomial distribution defined by the input tensor.
+
+ Parameters
+ ----------
+ gen : ThreefryKey
+ Generator state. Can be create with :py:func:`tvm.relay.threefry_key`. This should not be
+ reused in another function, otherwise random numbers will be repeated.
+
+ probs: Tensor[(input_rows, indices), float]
Review Comment:
Replacing negative values with zero sounds perfectly reasonable to me, as long as it's documented. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r937205819
##########
python/tvm/topi/random/kernel.py:
##########
@@ -602,3 +602,57 @@ def normal(gen, mean, scale, out_shape, out_dtype):
)
return new_gen, random_values
+
+
+def multinomial(gen, probs, num_samples):
+ """Draw samples from a multinomial distribution defined by the input tensor.
+
+ Parameters
+ ----------
+ gen : ThreefryKey
+ Generator state. Can be create with :py:func:`tvm.relay.threefry_key`. This should not be
+ reused in another function, otherwise random numbers will be repeated.
+
+ probs: Tensor[(input_rows, indices), float]
Review Comment:
I dont think there's a good way to check for negative values at runtime without introducing control flow (which we'd like to avoid). Do you think it's a fair interpretation to say that incoming negative values should have 0% chance of being selected? If so, I could insert a `clip` before processing the data to replace negative values with 0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936902713
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
Is there a good way to do this without potentially introducing flakiness? I guess we could use a fixed seed. Would that be satisfactory?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936912331
##########
python/tvm/topi/random/kernel.py:
##########
@@ -602,3 +602,57 @@ def normal(gen, mean, scale, out_shape, out_dtype):
)
return new_gen, random_values
+
+
+def multinomial(gen, probs, num_samples):
+ """Draw samples from a multinomial distribution defined by the input tensor.
+
+ Parameters
+ ----------
+ gen : ThreefryKey
+ Generator state. Can be create with :py:func:`tvm.relay.threefry_key`. This should not be
Review Comment:
Nit: "create" -> "created"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on PR #12284:
URL: https://github.com/apache/tvm/pull/12284#issuecomment-1207140053
@tkonolige can you give this another look. I think its all set to merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936912646
##########
python/tvm/topi/random/kernel.py:
##########
@@ -602,3 +602,57 @@ def normal(gen, mean, scale, out_shape, out_dtype):
)
return new_gen, random_values
+
+
+def multinomial(gen, probs, num_samples):
+ """Draw samples from a multinomial distribution defined by the input tensor.
+
+ Parameters
+ ----------
+ gen : ThreefryKey
+ Generator state. Can be create with :py:func:`tvm.relay.threefry_key`. This should not be
+ reused in another function, otherwise random numbers will be repeated.
+
+ probs: Tensor[(input_rows, indices), float]
+ A tensor containing the probabilities to sample from. Each value represents the
+ probability of choosing its corresonding index. If a tensor is provided, the last dimension
Review Comment:
Nit: "corresonding" -> "corresponding"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jwfromm commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
jwfromm commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r939213908
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
Thanks for this tip. I added a chisquare test which confirms that the behavior of this function is expected.
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
Thanks for this tip. I added a chisquared test which confirms that the behavior of this function is expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936925295
##########
python/tvm/topi/random/kernel.py:
##########
@@ -602,3 +602,57 @@ def normal(gen, mean, scale, out_shape, out_dtype):
)
return new_gen, random_values
+
+
+def multinomial(gen, probs, num_samples):
+ """Draw samples from a multinomial distribution defined by the input tensor.
+
+ Parameters
+ ----------
+ gen : ThreefryKey
+ Generator state. Can be create with :py:func:`tvm.relay.threefry_key`. This should not be
+ reused in another function, otherwise random numbers will be repeated.
+
+ probs: Tensor[(input_rows, indices), float]
Review Comment:
Traditionally, a multinomial distribution would enforce that these probabilities are all non-negative. It doesn't look like you're enforcing that condition here. Is that intentional? If so, please add a comment describing what should happen in the case of negative probabilities.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936918746
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
You could generate a "large" sample of at least 10,000 values and then use a chi-squared test (scipy.stats.chisquare). You'd look at the p-value from that chi-squared test and compare it to an acceptably low threshold for flakiness -- for example, have this unit test fail if the p-value is smaller than 1e-6, which should only happen by chance in one run per million.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
tkonolige commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936845448
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
Could you do some rough checking of expected value and variance of the distribution. It's always hard to tell if these random things are implemented correctly, but I think this would help.
##########
python/tvm/relay/frontend/pytorch.py:
##########
@@ -3386,6 +3386,19 @@ def trilu(self, inputs, input_types, mode):
upper = True if mode == "triu" else False
return _op.trilu(data, k, upper)
+ def multinomial(self, inputs, input_types):
+ probs = inputs[0]
+ num_samples = inputs[1]
+ replacement = inputs[2] if inputs[2] else True
+ assert not (
+ replacement is False and num_samples > 1
+ ), "Multinomial without replacement is not yet supported."
+ seed = np.random.randint(1e6)
Review Comment:
Ideally there would be one seed that we pass through the entire graph that is set or initialized at runtime. But I don't think we have the infrastructure for that yet. This is fine for now but maybe you could add a comment about how to improve this in the future?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] octoJon commented on a diff in pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
octoJon commented on code in PR #12284:
URL: https://github.com/apache/tvm/pull/12284#discussion_r936918746
##########
tests/python/topi/python/test_topi_prng.py:
##########
@@ -157,8 +185,27 @@ def test_uniform(target, dev):
assert np.max(rands) <= 10.0
+@tvm.testing.parametrize_targets
+def test_multinomial(target, dev):
+ def _verify_multinomial(size, num_samples):
Review Comment:
You could generate a "large" sample of at least 10,000 values and then use a chi-squared test (scipy.stats.chisquare). You'd look at the p-value from that chi-squared test and compare it to an acceptably low threshold for flakiness -- for example, have this unit test fail if the p-value is smaller than 1e-6, which should only happen by chance in one run per million.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] sfvaroglu commented on pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
sfvaroglu commented on PR #12284:
URL: https://github.com/apache/tvm/pull/12284#issuecomment-1203674033
LGTM, thanks @jwfromm! Would be nice to have this in the onnx importer, too :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige merged pull request #12284: [Relay][Op] Multinomial
Posted by GitBox <gi...@apache.org>.
tkonolige merged PR #12284:
URL: https://github.com/apache/tvm/pull/12284
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org