finch-tensor / finch-tensor-python

Sparse and Structured Tensor Programming in Python
MIT License
8 stars 3 forks source link

Unexpected `random` behavor #33

Closed mtsokol closed 6 months ago

mtsokol commented 6 months ago

Hi @willow-ahrens @hameerabbasi,

I just noticed that finch.random(..., random_state=...) isn't providing reproducible tensors for a fixed random state.

with sparse.Backend(backend=sparse.BackendType.Finch):
    result = sparse.random((3,3), density=1.0, random_state=0)
    print(result.todense())

Should give the same result for multiple runs but it doesn't. I think the reason is that:

using Random: default_rng
using Finch

rng = default_rng(0)
fsprand(rng, 5, 5)  # different output on multiple runs

rng = default_rng(0)
randn(rng, 4)

Also gives different results. The default_rng and underlying TaskLocalRNG (https://docs.julialang.org/en/v1/stdlib/Random/#Random.TaskLocalRNG) don't accept seed parameter (there could be at least a warning).

In finch-tensor in random we could replace default_rng with Xoshiro as it supports seed parameter and provides reproducible runs:

using Random
using Finch

rng = Xoshiro(0)
fsprand(rng, 5, 5)

rng = Xoshiro(0)
fsprand(rng, 5, 5)  # same output

WDYT?

willow-ahrens commented 6 months ago

I think that https://docs.julialang.org/en/v1/stdlib/Random/#Random.seed! should be able to seed the default_rng, but the stream of numbers it produces will not be the same across julia versions, right? I have no complaints with different RNGs, but I think the default rng is currently xoshiro. The challenge with rng is that often the generator changes with new updates. There's a StableRNG julia package with a stable random number generator across versions. I think we may simply want to avoid testing in a way that requires stability across versions.

willow-ahrens commented 6 months ago

You may need to call seed!(rng, 0) instead of rng(0)

mtsokol commented 6 months ago

I don't expect stability across versions for sure. Only for consecutive runs.

You may need to call seed!(rng, 0) instead of rng(0)

Sounds good to me - let me update the PR.

mtsokol commented 6 months ago

Fixed in https://github.com/willow-ahrens/finch-tensor/pull/34