Fixed #1153 Track Random Seed

[seanlaw]

Fixed #1153 and #707

Also see #1112

Pull Request Checklist

Below is a simple checklist but please do not hesitate to ask for assistance!

• Read our Contributing Guide
• Referenced a Github issue (or create one if one doesn't already exist)
• Left a meaningful comment on the original Github issue to discuss the detailed approach for your contribution
• Forked, cloned, and checkedout the newest version of the code
• Created a new branch
• Made necessary code changes
• Installed black (i.e., python -m pip install black or conda install -c conda-forge black)
• Installed flake8 (i.e., python -m pip install flake8 or conda install -c conda-forge flake8)
• Installed pytest-cov (i.e., python -m pip install pytest-cov or conda install -c conda-forge pytest-cov)
• Ran black --exclude=".*\.ipynb" --extend-exclude=".venv" --diff ./ in the root stumpy directory
• Ran flake8 --extend-exclude=.venv ./ in the root stumpy directory
• Ran ./setup.sh dev && ./test.sh in the root stumpy directory

[gitnotebooks]

Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1154

[seanlaw]

Currently, tests/test_floss.py::test_cac is failing because the stumpy/floss.py::_iac function is modified to use our custom rng.RNG:

    with rng.fix_seed(seed):
        I = rng.RNG.randint(0, width, size=width, dtype=np.int64)
        if bidirectional is False:  # Idealized 1-dimensional matrix profile index
            I[:-1] = width
            for i in range(width - 1):
                I[i] = rng.RNG.randint(i + 1, width, dtype=np.int64)

        target_AC = _nnmark(I)

        params = np.empty((n_iter, 2), dtype=np.float64)
        for i in range(n_iter):
            hist_dist = scipy.stats.rv_histogram(
                (target_AC, np.append(np.arange(width), width))
            )
            data = hist_dist.rvs(size=n_samples)
            a, b, c, d = scipy.stats.beta.fit(data, floc=0, fscale=width)

            params[i, 0] = a
            params[i, 1] = b

However, internally, the hist_dist uses the default np.random random number generator to sample from the desired histogram distribution rather than using our custom rng.RNG. To remedy this, we can force hist_dist to use our custom rng.RNG by adding hist_dist.random_state = rng.RNG and the result looks like:

with rng.fix_seed(seed):
        I = rng.RNG.randint(0, width, size=width, dtype=np.int64)
        if bidirectional is False:  # Idealized 1-dimensional matrix profile index
            I[:-1] = width
            for i in range(width - 1):
                I[i] = rng.RNG.randint(i + 1, width, dtype=np.int64)

        target_AC = _nnmark(I)

        params = np.empty((n_iter, 2), dtype=np.float64)
        for i in range(n_iter):
            hist_dist = scipy.stats.rv_histogram(
                (target_AC, np.append(np.arange(width), width))
            )
            hist_dist.random_state = rng.RNG
            data = hist_dist.rvs(size=n_samples)
            a, b, c, d = scipy.stats.beta.fit(data, floc=0, fscale=width)

            params[i, 0] = a
            params[i, 1] = b

[seanlaw]

With our new setup for tracking the random seed, at the start of each unit test file, you'll see:

conftest.py: rng.set_seed(3032615292)

So, to reproduce all of the tests, you simply need to go into the conftest.py file and replace the random seed inside of:

def pytest_configure(config):
    """
    Called after command line options have been parsed
    and all plugins and initial conftest files been loaded.
    """
    # rng.set_seed(seed)  # Replace this with your failed unit test seed

    print(f"conftest.py: rng.set_seed({rng.SEED})")

This should make reproducing failed tests locally a lot easier! And then revert the conftest.py file when you're done (i.e., remove the seed).

Note that all of this is still using the old "legacy numpy random number generator" and so all of our tests (that require a specific seed) are unchanged.

[seanlaw]

@NimaSarajpoor It is ready to be reviewed at your earliest convenience

[NimaSarajpoor]

@NimaSarajpoor It is ready to be reviewed at your earliest convenience

I will look into this PR within the next few days.

[NimaSarajpoor]

@seanlaw
You made a couple of attempts before and you finally did it! I left a few comments for your consideration.

[NimaSarajpoor]

Suggested change

	SEED = np.random.randint(1, 4_294_967_295, dtype=np.uint32)
	SEED = np.random.randint(1, 4_294_967_296, dtype=np.uint32)

Next line shows np.random.RandomState(seed=SEED). The numpy's doc provides the following statement regarding the value of seed:

Values can be any integer between 0 and 2**32 - 1 inclusive.

Since the value of high in np.random.randint is excluded (see doc), the param high should be set to 2**32 == 4_294_967_296

[NimaSarajpoor]

Isn't it better to do assert seed == 0 instead?

What if seed=1 and init_seed = 5? The current assertion does not fail. BUT, I believe it should fail because seed is expected to be zero here because of passing 0 to rng.fix_seed.

[NimaSarajpoor]

To be consistent with the context manager fix_state:

Suggested change

	state = RNG.get_state()
	curr_state = RNG.get_state()

[NimaSarajpoor]

IIUC, the RNG state will be set to a fixed state in this context manager (similar to what fix_state does), and that state is associated with the seed. Is that correct?

[NimaSarajpoor]

When you have a seed, it might be easier to remember that one can use fix_seed to fix the seed (and state). Is that why you did not refactor fix_seed and fix_state?

[NimaSarajpoor]

Can you please elaborate the last sentence?

If you are picking a random seed directly before calling fix_seed then you probably want to use fix_state instead!