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

Fix a scatter eager_subs issue #517

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Fix a scatter eager_subs issue #517

merged 3 commits into from
Apr 6, 2021

Conversation

fehiepsi
Copy link
Member

@fehiepsi fehiepsi commented Apr 3, 2021

Pair-coded with @eb8680 @fritzo @ordabayevy

This bug is detected while running a test for infer_discrete with NumPyro scan

funsor/terms.py Outdated
new_subs = []
for name, sub in self.subs:
if name in subs:
assert isinstance(subs[name], Variable)
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove this assertation seem to resolve the Slice substitution issue

(there is another issue happens in test_scan_hmm that @_get_support_value.register(funsor.cnf.Contraction) couldn't extract the delta_terms - but I guess that issue is not related to this)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess what's happening is that there can be bound variables of Scatter that share names with the fresh variables, and we need to implement a better _alpha_convert method that properly handles this case. Rather than just removing the assertion, does changing this to

if name in subs and isinstance(subs[name], Variable):
    new_subs.append((subs[name].name, sub))

fix the downstream error with Slice? It's still not correct in general, but it might be enough to unblock you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it does resolving the Slice issue (but the test still fails by the _get_support_value issue mentioned above).

@eb8680 eb8680 merged commit 057fb8b into master Apr 6, 2021
@eb8680 eb8680 deleted the scatter-bug branch April 6, 2021 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants