Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Reorder internal convolution and pooling operators for readability #279

Merged
merged 2 commits into from
Jun 23, 2019

Conversation

jon-tow
Copy link
Contributor

@jon-tow jon-tow commented Jun 22, 2019

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.

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jon-tow
Copy link
Contributor Author

jon-tow commented Jun 22, 2019

Glad to help!

@googlebot
Copy link

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 cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@eaplatanios
Copy link
Contributor

Note that the renaming changes I suggested will also require you to change the references to these VJP functions from other places.

@rxwei
Copy link
Contributor

rxwei commented Jun 22, 2019

😕 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.

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?

@eaplatanios
Copy link
Contributor

Yes sure. I confirm that I am okay with my commits being contributed to this PR.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@jon-tow
Copy link
Contributor Author

jon-tow commented Jun 22, 2019

@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 :)

@rxwei rxwei merged commit aeb2874 into tensorflow:master Jun 23, 2019
@jon-tow jon-tow deleted the operators/nn-refactor branch June 23, 2019 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants