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

Add ops.getslice for complex indexing by int,slice,None,Ellipsis #555

Merged
merged 5 commits into from
Sep 27, 2021

Conversation

fritzo
Copy link
Member

@fritzo fritzo commented Sep 25, 2021

Addresses #556
Blocking #553

This adds ops.getslice to perform fancy slice operations like x[..., 0, ::2, None, :], in particular supporting nontrivial slicing, and supporting unsqueezing via None. These fancier slicing ops will be needed for symbolic Gaussians #556. Note this now distinguishes between ops.getitem which supports "advanced indexing" where funsors can appear in the index, from ops.getslice which supports only indexing of ground values. Numpy distinguishes semantics of these two types of indexing, and it is much easier to fully implement ground indexing (which I believe this PR does for ops.getitem).

Tested

  • unit tests
  • pass existing tests

@fritzo fritzo added the WIP label Sep 25, 2021
@fritzo fritzo marked this pull request as ready for review September 25, 2021 14:29
@fritzo fritzo changed the title Add an ops.getslice for more complex eager indexing Add ops.getslice for complex indexing by int,slice,None,Ellipsis Sep 25, 2021
@fritzo fritzo requested a review from eb8680 September 25, 2021 14:43
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.

Code looks good, but can you add a couple of tests for Lambda with multiple-value indices to exercise eager_getslice_lambda?

funsor/terms.py Outdated
if head != slice(None):
expr = expr(**{x.var.name: head})
if x.var.name not in expr.inputs:
return expr
Copy link
Member

Choose a reason for hiding this comment

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

What if tail is not empty here?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed

@fritzo
Copy link
Member Author

fritzo commented Sep 27, 2021

ok, I've added some simple tests for multiple value indices

@fritzo fritzo merged commit 86d4a22 into master Sep 27, 2021
@fritzo fritzo deleted the getslice-op branch September 27, 2021 18:51
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