[AutoDiff] Remove '_Differentiable.zeroTangentVectorInitializer'. #35329

rxwei · 2021-01-09T01:48:43Z

Remove _Differentiable.zeroTangentVectorInitializer to address the feedback on the proposal thread. The corresponding change has already been made in the proposal.

Removed components:

zeroTangentVectorInitializer and zeroTangentVector in Differentiable, Array, Optional, Float, Double, Float80, and SIMD types.
zeroTangentVectorInitializer synthesis logic in Differentiable derived conformances.

rxwei · 2021-01-09T01:52:15Z

@swift-ci please test

Remove `_Differentiable.zeroTangentVectorInitializer` to address the feedback on the [proposal thread](https://forums.swift.org/t/differentiable-programming-for-gradient-based-machine-learning/42147). The corresponding change has already been made in the [proposal](https://github.com/rxwei/swift-evolution/blob/autodiff/proposals/0000-differentiable-programming.md). Removed components: - `zeroTangentVectorInitializer` and `zeroTangentVector` in `Differentiable`, `Array`, `Optional`, `Float`, `Double`, and `Float80`. - `zeroTangentVectorInitializer` synthesis logic in `Differentiable` derived conformances.

rxwei · 2021-01-09T02:01:34Z

@swift-ci please test

dan-zheng

Nice simplification!

Regarding API removal: I think there exists usages of Differentiable.zeroTangentVectorInitializer or Differentiable.zeroTangentVector computed property in projects using differentiable programming like tensorflow/swift-models and SwiftFusion. I believe the usages can be fixed, so proceeding with removal sounds good.

rxwei · 2021-01-09T02:23:33Z

Regarding API removal: I think there exists usages of Differentiable.zeroTangentVectorInitializer or Differentiable.zeroTangentVector computed property in projects using differentiable programming like tensorflow/swift-models and SwiftFusion. I believe the usages can be fixed, so proceeding with removal sounds good.

The existing usage seems very light and can be fixed by adding a declaration of zeroTangentVector (not zeroTangentVectorInitializer).

Existing occurrences of zeroTangentVectorInitializer will not be affected since all occurrences are just declaring it, not using it.

➜  SwiftFusion git:(main) grep -nrwi "zeroTangentVectorInitializer" .
./Sources/SwiftFusion/Core/Tuple+Vector.swift:46:  public var zeroTangentVectorInitializer: () -> TangentVector {
./Sources/SwiftFusion/Core/Tuple+Vector.swift:58:  public var zeroTangentVectorInitializer: () -> TangentVector {
./Sources/SwiftFusion/Core/Dictionary+Differentiable.swift:19:  public var zeroTangentVectorInitializer: () -> TangentVector {
./Sources/SwiftFusion/Inference/ArrayBuffer+Differentiable.swift:33:  public var zeroTangentVectorInitializer: () -> TangentVector {
./Sources/SwiftFusion/Inference/ArrayBuffer+Differentiable.swift:39:  // DWA TODO: replace this with the use of zeroTangentVectorInitializer
./Sources/SwiftFusion/Inference/AnyArrayBuffer+Differentiable.swift:25:  public var zeroTangentVectorInitializer: () -> TangentVector {
➜  swift-apis git:(main) grep -nrwi "zeroTangentVectorInitializer" .
./Sources/TensorFlow/Core/Tensor.swift:708:  public var zeroTangentVectorInitializer: () -> TangentVector {
➜  swift-models git:(main) grep -nrwi "zeroTangentVectorInitializer" .
./Models/Text/WordSeg/Model.swift:343:  // (`Differentiable.zeroTangentVectorInitializer`) instead of static zeros

zeroTangentVector is the one that's actually being used. It looks like you can just define a zeroTangentVector for any type that's missing it.

➜  SwiftFusion git:(main) grep -nrwi "zeroTangentVector" .
./Tests/SwiftFusionTests/Core/Dictionary+DifferentiableTests.swift:72:  /// Test the `Differentiable` `zeroTangentVector` requirement.
./Tests/SwiftFusionTests/Core/Dictionary+DifferentiableTests.swift:75:      ([:] as [String: Double]).zeroTangentVector,
./Tests/SwiftFusionTests/Core/Dictionary+DifferentiableTests.swift:79:      (["a": 1] as [String: Double]).zeroTangentVector,
./Tests/SwiftFusionTests/Geometry/PinholeCameraTests.swift:82:        calibration: K.zeroTangentVector),
./Tests/SwiftFusionTests/Geometry/PinholeCameraTests.swift:87:        calibration: K.zeroTangentVector),
./Sources/SwiftFusion/Core/LieGroup.swift:170:    return pullback(at: v.zeroTangentVector, in: { self.Adjoint($0) })(v)
./Sources/SwiftFusion/Core/Tuple+Vector.swift:59:    { .init(head: head.zeroTangentVector, tail: tail.zeroTangentVector) }
./Sources/SwiftFusion/Core/Dictionary+Differentiable.swift:20:    { mapValues { v in v.zeroTangentVector } }
./Sources/SwiftFusion/Core/Manifold.swift:103:      pullback(at: zeroTangentVector) { self.coordinateStorage.retract($0) }
./Sources/SwiftFusion/Image/Patch.swift:50:      var dBox = region.zeroTangentVector
./Sources/SwiftFusion/Inference/ArrayBuffer+Differentiable.swift:43:      .init(vs.lazy.map { $0.zeroTangentVector })
./Sources/SwiftFusion/Inference/AnyArrayBuffer+Vector.swift:107:    (lhs - rhs, { x in (x, x.zeroTangentVector - x) })
./Sources/SwiftFusion/Inference/AnyArrayBuffer+Vector.swift:125:    return ((), { x in x.zeroTangentVector - x })
./Sources/SwiftFusion/Inference/ArrayBuffer+Tensor.swift:139:    (lhs - rhs, { x in (x, x.zeroTangentVector - x) })
./Sources/SwiftFusion/Inference/ArrayBuffer+Tensor.swift:179:    return ((), { x in x.zeroTangentVector - x })
./Sources/SwiftFusion/Geometry/PinholeCamera.swift:81:          TangentVector(wTc: dpose, calibration: calibration.zeroTangentVector),
➜  swift-apis git:(main) grep -nrwi "zeroTangentVector" .
./Tests/TensorFlowTests/TensorTests.swift:126:    XCTAssertEqual(tensor.zeroTangentVector, Tensor(zeros: shape))
./Tests/TensorFlowTests/TensorTests.swift:132:    XCTAssertEqual(model.zeroTangentVector, .init(tensor: Tensor(zeros: shape)))
➜  swift-models git:(main) grep -nrwi "zeroTangentVector" .
./Tests/SupportTests/AnyLayerTests.swift:128:        // XCTAssertEqual(erased.zeroTangentVector, AnyLayerTangentVector(original.zeroTangentVector))
./Tests/SupportTests/AnyLayerTests.swift:129:        // XCTAssertEqual(AnyLayerTangentVector(original.zeroTangentVector), erased.zeroTangentVector)
./Models/Text/WordSeg/Lattice.swift:167:    let elementZero = positions[index].zeroTangentVector

dan-zheng · 2021-01-09T03:50:09Z

Thanks a lot for investigating!

zeroTangentVector is the one that's actually being used. It looks like you can just define a zeroTangentVector for any type that's missing it.

I wonder if this is actually doable?

Maybe: if concrete Tensor values with desired per-instance information (shape, device) are available for manually defining zeroTangentVector.
Maybe not: if it requires peering through types' stored properties to get the Tensors - this is why conformance derivation does work.

I think in the most general/inconvenient case, we can define a protocol refining Differentiable with var zeroTangentVectorInitializer and manually conform types to them. This would support the "maybe not" case with nested types.

Do you have any thoughts?

rxwei · 2021-01-09T06:00:21Z

What are the problems you are trying to solve? Is it really the right thing to keep the "elementwise per-instance zero" semantics in your libraries given that the AD transform has never supported zeroTangentVectorInitializer or zeroTangentVector?

dan-zheng · 2021-01-09T06:03:30Z

I'm not sure, since I haven't looked into specific use sites of zeroTangentVector yet. Thanks for raising that point - avoiding the "elementwise per-instance zero" semantics would definitely be ideal if possible, for semantic and implementation simplicity.

rxwei · 2021-01-09T06:25:52Z

I'm not sure, since I haven't looked into specific use sites of zeroTangentVector yet. Thanks for raising that point - avoiding the "elementwise per-instance zero" semantics would definitely be ideal if possible, for semantic and implementation simplicity.

Hmm it's not really about "avoiding the 'elementwise per-instance zero' semantics" -- the semantics was never there in the first place because AD is only using TangentVector.zero for zeros. The existing occurrences seem to be only relying on the effect of existing derived conformances at manually written call sites. With or without this PR, automatically differentiated code aren't using zeroTangentVectorInitializer and thus have identical behavior.

dan-zheng · 2021-01-09T06:32:45Z

That makes sense. Maybe we're talking past each other a bit: I think use cases of zeroTangentVector in SwiftFusion are unrelated to AD, they just want an easy way to create per-instance zeros. Looking at specific zeroTangentVector usages would give insight on the impact of the zeroTangentVector API removal. I can ask SwiftFusion code owners about it next week!

rxwei · 2021-01-11T21:24:36Z

@marcrasi Let me know when you are ready for me to merge this.

dan-zheng · 2021-01-11T21:37:05Z

@rxwei We'll look across Swift for TensorFlow repositories and users to check whether removing Differentiable.zeroTangentVector is okay by the end of the week, if that's okay!

rxwei · 2021-01-11T21:43:54Z

Sounds good. Thank you.

rxwei · 2021-01-17T01:35:32Z

@dan-zheng Any update on this? I am hoping to merge this soon in order to get the implementation closer to the proposal.

dan-zheng · 2021-01-17T14:17:03Z

I'll look into it today, thanks.

Here's the tracking issue: borglab/SwiftFusion#261. I think no one working on SwiftFusion has had a chance to investigate yet.

Edit: @acoadmarmon volunteered to look into updating SwiftFusion tomorrow, hope that's okay!

rxwei · 2021-01-19T04:14:39Z

I'll look into it today, thanks.

Here's the tracking issue: borglab/SwiftFusion#261. I think no one working on SwiftFusion has had a chance to investigate yet.

Edit: @acoadmarmon volunteered to look into updating SwiftFusion tomorrow, hope that's okay!

Any update on this?

dan-zheng · 2021-01-20T15:18:19Z

@rxwei Thanks for your patience and your ample announcement & exploration of this API removal!

I think it would be fine to merge this PR anytime, users who are broken can keep using their older development snapshot toolchains. main development needn't be blocked in general, as you mentioned.

I'll communicate to the autodiff users that I'm in touch with (SwiftFusion folks) to follow the differentiable programming pitch thread more closely, so they can be responsible for handling potential API changes in the future. @acoadmarmon @ProfFan @dellaert

Thanks all! 😄

rxwei · 2021-01-20T18:44:15Z

Thanks for confirming. Let me know if you have any additional issues in the migration.

rxwei requested review from stephentyrone, joshnewnham, dan-zheng and marcrasi January 9, 2021 01:48

rxwei force-pushed the remove-zero-tan-init branch from a2627ce to c8a6f01 Compare January 9, 2021 02:01

dan-zheng approved these changes Jan 9, 2021

View reviewed changes

rxwei marked this pull request as ready for review January 9, 2021 02:23

dan-zheng mentioned this pull request Jan 11, 2021

Rewrite usages of compiler-synthesized Differentiable.zeroTangentVector borglab/SwiftFusion#261

Open

rxwei merged commit e3db926 into swiftlang:main Jan 20, 2021

rxwei deleted the remove-zero-tan-init branch January 20, 2021 18:45

BradLarson mentioned this pull request Jan 22, 2021

Removing zeroTangentVector test tensorflow/swift-apis#1181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AutoDiff] Remove '_Differentiable.zeroTangentVectorInitializer'. #35329

[AutoDiff] Remove '_Differentiable.zeroTangentVectorInitializer'. #35329

rxwei commented Jan 9, 2021 •

edited

Loading

rxwei commented Jan 9, 2021

rxwei commented Jan 9, 2021

dan-zheng left a comment

rxwei commented Jan 9, 2021 •

edited

Loading

dan-zheng commented Jan 9, 2021

rxwei commented Jan 9, 2021

dan-zheng commented Jan 9, 2021

rxwei commented Jan 9, 2021 •

edited

Loading

dan-zheng commented Jan 9, 2021 •

edited

Loading

rxwei commented Jan 11, 2021

dan-zheng commented Jan 11, 2021

rxwei commented Jan 11, 2021

rxwei commented Jan 17, 2021 •

edited

Loading

dan-zheng commented Jan 17, 2021 •

edited

Loading

rxwei commented Jan 19, 2021

dan-zheng commented Jan 20, 2021 •

edited

Loading

rxwei commented Jan 20, 2021

[AutoDiff] Remove '_Differentiable.zeroTangentVectorInitializer'. #35329

[AutoDiff] Remove '_Differentiable.zeroTangentVectorInitializer'. #35329

Conversation

rxwei commented Jan 9, 2021 • edited Loading

rxwei commented Jan 9, 2021

rxwei commented Jan 9, 2021

dan-zheng left a comment

Choose a reason for hiding this comment

rxwei commented Jan 9, 2021 • edited Loading

dan-zheng commented Jan 9, 2021

rxwei commented Jan 9, 2021

dan-zheng commented Jan 9, 2021

rxwei commented Jan 9, 2021 • edited Loading

dan-zheng commented Jan 9, 2021 • edited Loading

rxwei commented Jan 11, 2021

dan-zheng commented Jan 11, 2021

rxwei commented Jan 11, 2021

rxwei commented Jan 17, 2021 • edited Loading

dan-zheng commented Jan 17, 2021 • edited Loading

rxwei commented Jan 19, 2021

dan-zheng commented Jan 20, 2021 • edited Loading

rxwei commented Jan 20, 2021

rxwei commented Jan 9, 2021 •

edited

Loading

rxwei commented Jan 9, 2021 •

edited

Loading

rxwei commented Jan 9, 2021 •

edited

Loading

dan-zheng commented Jan 9, 2021 •

edited

Loading

rxwei commented Jan 17, 2021 •

edited

Loading

dan-zheng commented Jan 17, 2021 •

edited

Loading

dan-zheng commented Jan 20, 2021 •

edited

Loading