Skip to content

[AutoDiff] Derive 'EuclideanDifferentiable' vector view from members' vector views. #26890

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 28, 2019
Merged

[AutoDiff] Derive 'EuclideanDifferentiable' vector view from members' vector views. #26890

merged 1 commit into from
Aug 28, 2019

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Aug 28, 2019

@rxwei rxwei added the tensorflow This is for "tensorflow" branch PRs. label Aug 28, 2019
@rxwei rxwei requested a review from dan-zheng August 28, 2019 00:02
@dan-zheng dan-zheng changed the title [AutoDiff] Derive 'EuclideanDifferentiable' vector view from member's vector view. [AutoDiff] Derive 'EuclideanDifferentiable' vector view from members' vector views. Aug 28, 2019
dan-zheng
dan-zheng approved these changes Aug 28, 2019
View reviewed changes
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 if tests pass!

I didn't check for missed opportunities for converting T == T.TangentVector requirement to T: EuclideanDifferentiable. I wonder if tensorflow/swift-apis requires changes too?

lib/Sema/DerivedConformanceDifferentiable.cpp
// <property_name_1...>: self.<property_name_1>,
// <property_name_2...>: self.<property_name_2>,
// <property_name_1...>:
// self.differentiableVectorView.<property_name_1>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be:

    //   TangentVector.init(
    //     <property_name_1>:
    //        self.<property_name_1>.differentiableVectorView,
    //     <property_name_2>:
    //        self.<property_name_2>.differentiableVectorView,
    //     ...

self.<property_name_1>.differentiableVectorView rather than self.differentiableVectorView.<property_name_1>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Good catch.

@rxwei
Copy link
Contributor Author

rxwei commented Aug 28, 2019

tensorflow/swift-apis builds successfully, because this doesn't change Differentiable's conformance requirements.

@rxwei rxwei marked this pull request as ready for review August 28, 2019 01:17
@rxwei
Copy link
Contributor Author

rxwei commented Aug 28, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor Author

rxwei commented Aug 28, 2019

Merging this to resolve urgent blockers. Will address comments later.

@rxwei rxwei merged commit f34c92f into swiftlang:tensorflow Aug 28, 2019
@rxwei rxwei deleted the eucdiff branch August 28, 2019 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-zheng dan-zheng dan-zheng approved these changes

Assignees
No one assigned
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rxwei @dan-zheng