Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Sampling functions with structure_prior_from_depth/max_depth is not deterministic #29

Open
lfrommelt opened this issue Nov 28, 2024 · 0 comments · May be fixed by #32
Open

Bug: Sampling functions with structure_prior_from_depth/max_depth is not deterministic #29

lfrommelt opened this issue Nov 28, 2024 · 0 comments · May be fixed by #32

Comments

@lfrommelt
Copy link
Collaborator

How to reproduce:

prior=structure_prior_from_depth(depth)

print(prior)

restart runtime

prior=structure_prior_from_depth(depth)

print(prior)

At this point we can see two differently ordered dictionaries. Since the position of items in the prior dictionaries is used during sampling, this will lead to undeterministic behaviour despite seeding. I assume the

UserWarning: No hashed prior found. Sample frequencies may diverge from the prior. Consider burning this prior first.

warning might be related to this issue (if the hash that is being referred to is in fact the position as int), but I am not sure and I think this should be solved instead of warning about it, anyways.

Here is my recommended fix: Instead of making sure that the outputs of structure_prior_from[_max]_depth returns deterministic and ordered results, the sampling of equations should not depend on the ordering of (usually unordered) dictionaries. Since the dictionary/prior keys are strings, we can use the sorted strings as an arbitrary but deterministic ordering, by including the following line in sample.sample:

    _prior = copy.deepcopy(prior)
    _prior = {prior_type: {key: _prior[prior_type][key] for key in sorted(_prior[prior_type])} for prior_type in _prior.keys()}
    if max_depth is not None and depth is not None:

Or more explicitly: iterate over the priors and then do the dict comprehension individiually. Also, apparently nowadays any python dict is ordered, not only OrderedDicts, so after this point we don't have to worry anymore.

I'm not sure if there are additional chunks that need to be changed, but this fix worked pretty well for my own use-case.

Cheers,
Leonard

@lfrommelt lfrommelt linked a pull request Dec 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant