-
Notifications
You must be signed in to change notification settings - Fork 22
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
Convolution transpose #183
Conversation
6bfaa07
to
69e34ef
Compare
fbb5270
to
d338d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM and the tests pass, but as with a lot of NengoDL stuff, I don't know this material deep enough to comment on things like efficiency and correctness. Do you want to take a quick look as well @drasmuss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look through, some minor comments on efficiency/version handling below but all looks good other than that 👍
d338d0b
to
bee15de
Compare
OK, fixed those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixups LGTM 👍
7d0d54c - Upgrade black version to 21.12b0 378ad3d - Switch to new codecov uploader ab7f940 - Avoid bad astroid version
029d4fa
to
84e8dfa
Compare
93e6c6f
to
511aa4e
Compare
Squashed it all, and added one change. We moved those tests to a new file, so that custom allclose tolerance had to move, but also I found that I had to increase the tolerance a fair bit. This might be due to all the transposing exacerbating floating point rounding differences (though that doesn't sound plausible when I type it out), or simply the seed changed and this new seed is unlucky. Here's the failure that I was getting:
So it's a negligible difference in one array element. Probably just unlucky! |
Support the ConvTransposeInc operator added in nengo/nengo#1648.
Things might be a bit cleaner if we support
GeneralConvInc
instead ofConvInc
andConvTransposeInc
(then e.g. we could have oneGeneralConvSet
operator), but some code (e.g. operator merging) breaks if we don't have build methods for each operator class (if we try to do them only for a superclass).TODO: