Skip to content

[AutoDiff] Remove 'withoutDerivative(at:in:)' and 'withDerivative(_:)'. #36124

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

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Feb 24, 2021

These APIs are not included in the initial proposal. This PR moves them to DifferentiationUnittests.

Rename `move(along:)` to `move(by:)` based on the proposal feedback. The main argument for the change is that tangent vectors specify both a direction and a magnitude, whereas `along:` does not indicate that `self` is being moved by the specified magnitude.
@rxwei
Copy link
Contributor Author

rxwei commented Feb 24, 2021

Please review 67e84b6.

@rxwei
Copy link
Contributor Author

rxwei commented Feb 24, 2021

@swift-ci please smoke test and merge

Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

LGTM (seems like users of removed APIs can redefine in their own code, thanks to the non-hardcoded design)

Rename the argument label `in:` in `gradient(at:in:)`, `pullback(at:in:)`, etc to `of:`, as suggested in the [pitch thread](https://forums.swift.org/t/differentiable-programming-for-gradient-based-machine-learning/42147).
These APIs are not included in the initial proposal.
@rxwei rxwei force-pushed the remove-withderivative branch from 67e84b6 to 17cf272 Compare February 24, 2021 06:36
@rxwei
Copy link
Contributor Author

rxwei commented Feb 24, 2021

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 0b15060 into swiftlang:main Feb 24, 2021
@rxwei rxwei deleted the remove-withderivative branch February 24, 2021 17:37
@porterchild
Copy link

I'd just like to say that there are some Autodiff bugs that require workarounds using .withDerivative in my codebase (mostly related to SR-14218 and array differentiation) and I'm not sure I would have found workarounds without having withDerivative. It is also useful when I want to print gradients as they propagate as a 'poor man's debugger'.

There may be reasons (scope of the current proposal (?)) to remove it, and it is technically re-definable by users, but from a beginner's standpoint, I think withDerivative hugely increases usability, and it's very unlikely I would have thought to re-define it myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants