-
Notifications
You must be signed in to change notification settings - Fork 137
Reorder internal convolution and pooling operators for readability #279
Conversation
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.
LGTM. Thank you!
Glad to help! |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
Note that the renaming changes I suggested will also require you to change the references to these VJP functions from other places. |
CLA doesn't work automatically with directly-committed suggestions. For formality, @eaplatanios could you please confirm that you are okay with your commits being contributed to this PR? |
Yes sure. I confirm that I am okay with my commits being contributed to this PR. |
015050a
to
cb4e1d2
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@rxwei I had to remove some "merge branch" commits that cluttered the PR's commit history just in case it bothers you all as much as it bothers me :) |
Convolution and pooling functions were scattered around the NN.swift file along with their corresponding vjps. This PR gathers them in an organized way for good code hygiene. The only non-reordering change was a line break deletion.