Skip to content
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

[AutoDiff] Change Differentiable.moved(along:) to move(along:). #25437

Merged

Conversation

dan-zheng
Copy link
Contributor

Change Differentiable.moved(along:) to mutating func move(along:).
Update Differentiable derived conformances (logic and diagnostics).
Update tests.


Example:

struct Foo : Differentiable {
    var x, y: Float
  
    // Old: returns a new instance.
    // func moved(along direction: TangentVector) -> Foo {
    //     return Foo(x: x.moved(along: direction.x),
    //                y: y.moved(along: direction.y))
    // }
  
    // New: updates in-place.
    mutating func move(along direction: TangentVector) {
        x.move(along: direction.x)
        y.move(along: direction.y)
    }
}

This is important for upcoming Differentiable class support.
moved(along:) is problematic for classes because it returns a new instance, which is undesirable.
Instead, move(along:) updates classes in-place.

Derived conformances logic and diagnostics are updated.
One important change is: Differentiable derived conformances now generate a warning for all let stored properties, instead of only let stored properties with initial value. The reason is that synthesis of move(along:) requires all stored properties to be mutable.

Example:

struct Example: Differentiable {
    // These stored properties are fine.
    var x: Float
    var y = Float(1)

    // Warnings are emitted for these stored properties.
    // Synthesis (`TangentVector`, `move(along:)`, etc) ignores these properties.
    // They are implicitly marked with `@noDerivative`.
    let bad1: Float
    let bad2: Int

    // Example synthesis.
    // Intentionally ignores `AllDifferentiableVariables` because it will be removed soon.
    //
    // struct TangentVector: Differentiable, AdditiveArithmetic, VectorProtocol {
    //     var x: Float.AllDifferentiable
    //     var y: Float.AllDifferentiable
    // }
    //
    // mutating func move(along direction: TangentVector) {
    //     x.move(along: direction.x)
    //     y.move(along: direction.y)
    // }
}
example.swift:9:9: warning: synthesis of the 'Differentiable.move(along:)' requirement for 'Example' requires all stored properties to be mutable; use 'var' instead, or add an explicit '@noDerivative' attribute, or conform 'Example' to 'AdditiveArithmetic'
    let bad1: Float
        ^
    @noDerivative
example.swift:10:9: warning: stored property 'bad2' has no derivative because it does not conform to 'Differentiable'; add an explicit '@noDerivative' attribute, or conform 'Example' to 'AdditiveArithmetic'
    let bad2: Int
        ^
    @noDerivative

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Jun 13, 2019
@dan-zheng dan-zheng force-pushed the rename-differentiable-move-along branch from 61b405a to a6f4488 Compare June 13, 2019 21:38
Change `Differentiable.moved(along:)` to `mutating func move(along:)`.
This is important for upcoming `Differentiable` class support.

Update `Differentiable` derived conformances (logic and diagnostics).
Update tests.
@dan-zheng dan-zheng force-pushed the rename-differentiable-move-along branch from a6f4488 to bc8d3c7 Compare June 13, 2019 21:41
The `@noDerivative` fixit location was correct, but the location of the
warning marker `^` was incorrect.

Before:
```
    let bad2: Int
        ^
    @noDerivative
```

After:
```
    let bad2: Int
    ^
    @noDerivative
```
@dan-zheng dan-zheng force-pushed the rename-differentiable-move-along branch from 2ebcf5d to 5447767 Compare June 14, 2019 02:49
@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

@dan-zheng dan-zheng merged commit c1211a3 into swiftlang:tensorflow Jun 14, 2019
@dan-zheng dan-zheng deleted the rename-differentiable-move-along branch June 14, 2019 03:52
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