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 PES learning with sliced post #1317

Merged
merged 1 commit into from
Sep 22, 2017
Merged

Fix PES learning with sliced post #1317

merged 1 commit into from
Sep 22, 2017

Conversation

tbekolay
Copy link
Member

Motivation and context:
@celiasmith was having issues doing learning on a connection in which the post was an ObjView. This was failed when using a weight-based solver because we used the full encoder matrix. This PR adds a test for that case, and fixes the builder to slice the encoder matrix when the post object is an ObjView.

One discussion point:

Currently, this slices the post by slicing into the encoders signal. However, in builder/connections.py, we have a slice_signal helper function that is required for advanced indexing. I seem to recall that we talked about disallowing advanced indexing for Nengo objects, but I think it is still possible. Are we allowing advanced indexes?

How has this been tested?
Added a unit test which fails without this PR.

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jgosmann
Copy link
Collaborator

@jgosmann
Copy link
Collaborator

There is already some discussion about advanced indexing in #948. There is also at least one bug related to to post slices with advanced indexing (#947) which might be relevant to this PR which uses the post slice.

It might be good to come to a decision before on advanced indexing before reviewing/merging this PR. Or we can go ahead with the review process (as it fixes at least one use case), but should then ensure that we revisit this code once we properly allow or disallow advanced indexing (because this code fragment is not related to the rest of the indexing code, I fear that it might be forgotton).

@tbekolay
Copy link
Member Author

I added #948 to the dev meeting agenda for next week. Let's review this one in isolation for now since it fixes what I think is a bug, and the actual fix is only two lines (the rest is tests).

Copy link
Member

@drasmuss drasmuss left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Collaborator

@jgosmann jgosmann left a comment

Choose a reason for hiding this comment

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

I rebased onto master (tests weren't passing locally before that) and added a commit with explanation in the commit message.

This would fail when using a weight-based solver because we would
use the full encoder matrix. Now, we slice the encoder matrix
with the post slice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants