[Relay][Op] Multinomial #12284
[Relay][Op] Multinomial #12284
Conversation
|
@sfvaroglu can you take a look at this PR? |
|
LGTM, thanks @jwfromm! Would be nice to have this in the onnx importer, too :) |
| assert not ( | ||
| replacement is False and num_samples > 1 | ||
| ), "Multinomial without replacement is not yet supported." | ||
| seed = np.random.randint(1e6) |
There was a problem hiding this 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?
There was a problem hiding this 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.
| @@ -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): | |||
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this comment.
Thanks for this tip. I added a chisquared test which confirms that the behavior of this function is expected.
|
@tkonolige can you give this another look. I think its all set to merge. |
* Add multinomial operator. * Implemented Pytorch integration with multinomial. * Fixed test paramatrization and added onnx integration. * Add statistical testing. * Make get_type more flexible.
This PR introduces the
multinomialrandom operator. It's a neat adaptation ofrandom.uniformthat 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.