Skip to content

[AutoDiff] Deprecate Differentiable.AllDifferentiableVariables. #26527

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

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Aug 7, 2019

Deprecate the AllDifferentiableVariables associated type of the
Differentiable protocol.

AllDifferentiableVariables is not essential for differentiable
programming and was added as a workaround to enable key-path-based
machine learning optimizers: let parameters and gradients have the same type
(AllDifferentiableVariables == TangentVector) to enable joint key-path
iteration.

It is possible to implement key-path-based machine learning optimizers
via other means (do key-path-based operations on TangentVector, then
call Differentiable.move(along:) to perform update), so
AllDifferentiableVariables is no longer necessary.


Simplified Differentiable protocol:

/// A type that mathematically represents a differentiable manifold whose
/// tangent spaces are finite-dimensional.
public protocol Differentiable {
  /// A type representing a differentiable value’s derivatives.
  /// Mathematically, this is equivalent to the tangent bundle of the
  /// differentiable manifold represented by the differentiable type.
  associatedtype TangentVector: Differentiable & AdditiveArithmetic
    where TangentVector.TangentVector == TangentVector

  /// Moves `self` along the given direction. In Riemannian geometry,
  /// this is equivalent to exponential map, which moves `self` on the
  /// geodesic surface along the given tangent vector.
  mutating func move(along direction: TangentVector)
}

Resolves TF-707.

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Aug 7, 2019
Deprecate the `AllDifferentiableVariables` associated type and
`var allDifferentiableVariables` property of the `Differentiable` protocol.

`AllDifferentiableVariables` is not essential for differentiable
programming and was added as a workaround to enable key-path-based
machine learning optimizers: let parameters and gradients have same type
(`AllDifferentiableVariables == TangentVector`) to enable joint key-path
iteration.

It is possible to implement key-path-based machine learning optimizers
via other means (do key-path-based operations on `TangentVector`, then
call `Differentiable.move(along:)` to perform update), so
`AllDifferentiableVariables` is no longer necessary.

Resolves TF-707.
@dan-zheng dan-zheng force-pushed the deprecate-differentiable-alldiffvars branch from caf0769 to 36c5fcd Compare August 7, 2019 02:50
@@ -1952,26 +1952,6 @@ extension Array where Element : Differentiable {

public typealias TangentVector =
Array<Element.TangentVector>.DifferentiableView
public typealias AllDifferentiableVariables =
Copy link
Contributor

Choose a reason for hiding this comment

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

In a separate patch, Array.DifferentiableView should be replaced with a struct TangentVector as described in the design overview.

@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

@dan-zheng
Copy link
Contributor Author

CI weirdly fails with:

/Users/swiftci/jenkins/workspace/swift-PR-TensorFlow-macOS/tensorflow-swift-apis/Sources/TensorFlow/Loss.swift:290:12: error: the compiler is unable to type-check this expression in reasonable time; try breaking up the expression into distinct sub-expressions
    return reduction(maxLogitsWithZero - logits * labels + log(1 + exp(-abs(logits))))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

But I'm not able to reproduce this error locally. I'll break up the expression into two subexpressions nonetheless and try again.

- Remove remaining `var allDifferentiableVariables` references.
- Update checkout for tensorflow-swift-apis to fix compilation.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please clean test tensorflow

@dan-zheng
Copy link
Contributor Author

Oops, I missed failing AutoDiff tests.

Failing Tests (7):
    Swift(linux-x86_64) :: AutoDiff/leakchecking.swift
    Swift(linux-x86_64) :: AutoDiff/vtable_sil.swift
    Swift(linux-x86_64) :: AutoDiff/generics.swift
    Swift(linux-x86_64) :: AutoDiff/derived_differentiable.swift
    Swift(linux-x86_64) :: AutoDiff/control_flow.swift
    Swift(linux-x86_64) :: AutoDiff/e2e_differentiable_property.swift
    Swift(linux-x86_64) :: AutoDiff/derived_conformances.swift

Will fix tomorrow.

Add todo comment for TF-632: remove ad-hoc conformance logic for `TangentVector`
when synthesized member types can be extended.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng dan-zheng merged commit 2e582b9 into swiftlang:tensorflow Aug 7, 2019
@dan-zheng dan-zheng deleted the deprecate-differentiable-alldiffvars branch August 7, 2019 17:22
@dan-zheng
Copy link
Contributor Author

@rxwei
Copy link
Contributor

rxwei commented Aug 7, 2019

@BradLarson @sgugger

dan-zheng added a commit to dan-zheng/swift that referenced this pull request Aug 19, 2019
dan-zheng added a commit to ematejska/swift-1 that referenced this pull request Sep 10, 2019
`Differentiable.allDifferentiableVariables` deprecated in v0.5: swiftlang/swift#26527.
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