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

Implement affine pattern recognition for Gaussian substitution #285

Merged
merged 7 commits into from
Oct 22, 2019

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Oct 22, 2019

Addresses #72
Follows #284

This is step 2/3 in implementing affine recognition for Gaussian funsors. This PR:

  • implements Gaussian._eager_subs_affine().
  • refactors _find_gaps() to _find_intervals() for nonuniform block slicing in BlockVector and BlockMatrix.

Tested

  • added a unit test w/ a variety of substitutions and batch shapes
  • strengthened tests for BlockMatrix

assert issubclass(type(g_subs), GaussianMixture)
actual = g_subs(**grounding_subs)
expected = g(**ground_subs)(**grounding_subs)
assert_close(actual, expected, atol=1e-3, rtol=1e-4)
Copy link
Member

Choose a reason for hiding this comment

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

Finally, I can understand the purpose of each statement in this test. :D I will review the logic eager_subs_affine soon.

@fritzo fritzo mentioned this pull request Oct 22, 2019
Copy link
Member

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

This is great! The Gaussian math looks correct to me. The indexing and alignment code look correct as well but are somewhat intricate.

new_offset = new_offsets[old_k]
new_slice = slice(new_offset, new_offset + old_size)
subs_matrix[..., new_slice, old_slice] = \
torch.eye(old_size).expand(batch_shape + (-1, -1))
Copy link
Member

Choose a reason for hiding this comment

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

This more advanced slicing isn't covered by existing BlockMatrix tests in test_gaussian.py, right? Would it make sense to add a couple of test cases there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do.

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 advanced indexing was already covered, but the particular sparse sequence of entries was not, so I added a test for that.

g_subs = g(**subs)
assert issubclass(type(g_subs), GaussianMixture)
actual = g_subs(**grounding_subs)
expected = g(**ground_subs)(**grounding_subs)
Copy link
Member

Choose a reason for hiding this comment

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

This is a very satisfying test 🎉

Copy link
Member

@fehiepsi fehiepsi left a comment

Choose a reason for hiding this comment

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

Look great! Thank Fritz for adding me to review! I understand funsor much better now.

funsor/gaussian.py Outdated Show resolved Hide resolved
test/test_gaussian.py Show resolved Hide resolved
@fritzo
Copy link
Member Author

fritzo commented Oct 22, 2019

The indexing and alignment code ... are somewhat intricate.

Let's hope this is the last time we'll ever need to write this code 😄

@eb8680 eb8680 merged commit 2a4b574 into master Oct 22, 2019
@eb8680 eb8680 deleted the affine-gaussian-2 branch October 22, 2019 22:02
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.

3 participants