Skip to content

Conversation

@rxwei
Copy link
Contributor

@rxwei rxwei commented Mar 2, 2019

For demo purposes.

@rxwei rxwei added the tensorflow This is for "tensorflow" branch PRs. label Mar 2, 2019
@rxwei rxwei requested a review from marcrasi March 2, 2019 02:04
Copy link

Choose a reason for hiding this comment

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

ModifyGradientOfSum?

Copy link

Choose a reason for hiding this comment

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

withGradient(_ body: @escaping (CotangentVector) -> CotangentVector) seems like a more natural API at first glance. Do you have a reason to do it this way?

Copy link

Choose a reason for hiding this comment

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

Discussed in person

Copy link
Contributor Author

@rxwei rxwei Mar 2, 2019

Choose a reason for hiding this comment

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

I think my core rationale is that withXXX APIs in Swift usually return whatever the closure returns, e.g. withUnsafePointer(_:) and withoutAcutallyEscaping(_:), so x.withGradient { $0 + 1 } would syntactically indicate to a Swift programmer that this is "returning the gradient plus one in the forward pass".

Forbidding a return value makes it slightly less misleading. We can totally change this later.

@rxwei
Copy link
Contributor Author

rxwei commented Mar 2, 2019

@swift-ci please test tensorflow

@rxwei rxwei merged commit 2d2ae43 into swiftlang:tensorflow Mar 2, 2019
@rxwei rxwei deleted the custom-grad branch March 2, 2019 03:53
rxwei added a commit to rxwei/swift that referenced this pull request May 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tensorflow This is for "tensorflow" branch PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants