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

Ensure alpha conversion always runs when a new variable is bound #414

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

eb8680
Copy link
Member

@eb8680 eb8680 commented Dec 28, 2020

This PR fixes a longstanding alpha-conversion edge case by guaranteeing that newly bound variables are always alpha-renamed, which is currently not the case due to the way alpha-conversion interacts with cons-hashing. For context, this issue turned out to be the root cause of some strange, difficult-to-debug errors that came up when I tested funsor.adjoint with funsor.sum_product.modified_partial_sum_product. I imagine @ordabayevy will run into this at some point, so think of this PR as preemptive unblocking.

Recall that alpha conversion in Funsor is implemented with name-mangling of subexpressions in reflect: whenever a variable is newly bound in a Funsor expression, we append to its name a unique suffix of the form f"__BOUND_{identifier}".

However, to ensure that expressions that bind variables are cons-hashed, we have to avoid mangling names twice. This is currently achieved by only mangling bound variables whose names do not already contain a __BOUND_ suffix, a heuristic that can sometimes break down when subexpressions containing mangled inputs are temporarily exposed for further processing. This PR switches to always mangling bound variables and should guarantee that alpha-conversion always behaves as expected.

Tested:

  • Added a simple regression test that fails on master
  • Exercised explicitly by existing tests in test_alpha_conversion.py and implicitly in any test that compares identities of expressions with bound variables (x1 is x2)

@eb8680 eb8680 requested a review from fritzo December 28, 2020 06:29
@eb8680 eb8680 removed the request for review from fritzo December 28, 2020 07:19
@eb8680 eb8680 added the WIP label Dec 28, 2020

# we also make the old cons cache_key point to the new mangled value.
# this guarantees that alpha-conversion only runs once for this expression.
cls._cons_cache[cache_key] = result
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line ensures that _alpha_mangle does not break cons-hashing.

@eb8680
Copy link
Member Author

eb8680 commented Dec 28, 2020

The CI failures are RecursionErrors in examples/hmm.py. After some investigation it seems like there's an odd interaction happening between substitute and funsor.optimize.apply_optimizer that's causing the stack of interpreters to blow up. I don't think it's related to the changes in this PR; they just happened to make it more obvious. I'll have to fix that first before this can be merged.

@eb8680 eb8680 added the Blocked Blocked by other issues label Dec 28, 2020
@eb8680 eb8680 removed the Blocked Blocked by other issues label Feb 22, 2021
@eb8680
Copy link
Member Author

eb8680 commented Feb 22, 2021

there's an odd interaction happening between substitute and funsor.optimize.apply_optimizer that's causing the stack of interpreters to blow up

Update: I believe I have fixed this by forcing alpha-renaming substitution to run under reflect, which I believe is always correct. The same example is still giving a RecursionError, but I think the root cause has changed. Still investigating...

@eb8680 eb8680 added bug Something isn't working refactor labels Mar 22, 2021
@eb8680
Copy link
Member Author

eb8680 commented May 12, 2021

This recent paper seems quite relevant, although it solves a slightly different problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working refactor WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant