Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Deprecate Differentiable.AllDifferentiableVariables. #419

Merged
merged 3 commits into from
Aug 7, 2019
Merged

Deprecate Differentiable.AllDifferentiableVariables. #419

merged 3 commits into from
Aug 7, 2019

Conversation

dan-zheng
Copy link
Member

Remove usages of AllDifferentiableVariables and
var allDifferentiableVariables.

Friend PR: swiftlang/swift#26527

@dan-zheng dan-zheng force-pushed the deprecate-differentiable-alldiffvars branch from 7f34040 to 8911fd8 Compare August 7, 2019 02:49
@dan-zheng dan-zheng requested a review from rxwei August 7, 2019 02:50
@dan-zheng
Deprecate Differentiable.AllDifferentiableVariables.
bcdc695 
Remove usages of `AllDifferentiableVariables` and
`var allDifferentiableVariables`.
@dan-zheng dan-zheng force-pushed the deprecate-differentiable-alldiffvars branch from 8911fd8 to bcdc695 Compare August 7, 2019 02:50
rxwei
rxwei approved these changes Aug 7, 2019
View reviewed changes
Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

I'm excited to see this step finally done! :)

@dan-zheng
Break up expression to fix type-checking.
bfaaf71 
Break up expression into two subexpressions to fix "compiler is unable to
type-check this expression in reasonable time" error.
@dan-zheng
Remove remaining uses of AllDifferentiableVariables.
12ad18d 
Remove remaining uses of `AllDifferentiableVariables` and
`allDifferentiableVariables`.
@dan-zheng
Copy link
Member Author

dan-zheng commented Aug 7, 2019 •
edited
Loading

All references should have been removed.

$ grep -nr "AllDifferentiableVariables" Sources Tests
# No results.

@Shashi456
Copy link
Contributor

Shashi456 commented Aug 7, 2019 •
edited
Loading

@dan-zheng there's a couple of `AllDifferentiableVariables" in the readme.

@dan-zheng
Copy link
Member Author

dan-zheng commented Aug 7, 2019 •
edited
Loading

@dan-zheng there's a couple of `AllDifferentiableVariables" in the readme.

I saw that, thanks! I'll fix that in a follow-up PR, since swiftlang/swift#26527 is sensitive to the commit version of tensorflow/swift-apis.

@Shashi456
Copy link
Contributor

@dan-zheng is the loss function refactoring necessary in this PR?

@dan-zheng
Copy link
Member Author

@dan-zheng is the loss function refactoring necessary in this PR?

I'm not sure what exactly you're referring to, but it sounds orthogonal. This patch is meant to be minimal, removing references to AllDifferentiableVariables/allDifferentiableVariables.

@dan-zheng
Copy link
Member Author

The friend PR swiftlang/swift#26527 has been merged.
apple/swift now points to a commit of tensorflow/swift-apis from this PR.

Shall we hold off on merging this PR until the CI toolchain has swiftlang/swift#26527? cc @rxwei

@rxwei
Copy link
Contributor

rxwei commented Aug 7, 2019

The CI toolchain is already broken unfortunately due to build failures (marcrasi@ is looking into this AFAIK). I think we should merge this.

@dan-zheng dan-zheng merged commit 085217d into master Aug 7, 2019
@dan-zheng dan-zheng deleted the deprecate-differentiable-alldiffvars branch August 7, 2019 19:44
@BradLarson BradLarson mentioned this pull request Aug 8, 2019
Merged
pschuh added a commit to pschuh/swift-apis that referenced this pull request Aug 8, 2019
@pschuh
Revert "Deprecate Differentiable.AllDifferentiableVariables. (tenso…
c8b5577 
…rflow#419)"

This reverts commit 085217d.

This broke CI. reverting so that a fix can happen.
pschuh added a commit to pschuh/swift-apis that referenced this pull request Aug 8, 2019
@pschuh
Fix Sequential Tests.
b2bd043 
Fix bug introduced in tensorflow#419.
@pschuh pschuh mentioned this pull request Aug 8, 2019
Merged
pschuh added a commit that referenced this pull request Aug 8, 2019
@pschuh
Fix Sequential Tests. (#424)
1b77875 
Fix bug introduced in #419.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Shashi456 Shashi456 Shashi456 left review comments

@rxwei rxwei rxwei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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