Skip to content

[AutoDiff][TF-1200] Adding derivatives for stdlib pow function. #30580

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
merged 5 commits into from
May 5, 2020

Conversation

vguerra
Copy link
Contributor

@vguerra vguerra commented Mar 23, 2020

Adding JVP and VJP function derivatives for pow function
defined in stdlib.

Resolves TF-1200.

@dan-zheng dan-zheng requested review from marcrasi and dan-zheng March 23, 2020 14:33
@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Mar 23, 2020
@vguerra vguerra force-pushed the tf-1200 branch 2 times, most recently from 1a7caab to 44d1a49 Compare April 19, 2020 16:10
@@ -38,17 +38,40 @@ func expectEqualWithTolerance<T>(_ expected: TestLiteralType, _ actual: T,
file: file, line: line)
}

func computeDividedDifference<T: BinaryFloatingPoint> (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use instead symmetric derivative formula to compute the numerical derivative, which gives a more precise result hence allowing to use lower values for ulps but some test cases for reminder broke, hence I dropped that change, but maybe we should use it in the future.


// pow
let eps:${T} = 0.01
let ulps:${T} = eps/eps.ulp
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here I compute the needed ulps for comparing values as the resulting numerical derivative depends on the ε used to compute it.

@vguerra
Copy link
Contributor Author

vguerra commented Apr 19, 2020

hello @dan-zheng, sorry that it took me this long to address your comments but I finally had time this weekend to address them.

I needed to rebase my PR due to conflicts, so I did :/.

Copy link

@marcrasi marcrasi left a comment

Choose a reason for hiding this comment

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

We have moved most of differentiation to the master branch now! Could you change this PR to be against the master branch? I think that it should rebase pretty cleanly now.

vguerra added 5 commits April 22, 2020 23:15
Adding JVP and VJP function derivatives for `pow` function
defined in stdlib.

Resolves TF-1200.
The versions of VJP and JVP match the results that TensorFlow
woudl give.

As well, this extends the test cases to include combinations of positive
and negative values and corner cases.
We now have 3 sections for the tests: negative base, 0 base and
positive base.
@vguerra vguerra changed the base branch from tensorflow to master April 22, 2020 21:25
@vguerra vguerra changed the title Adding derivatives for stdlib pow function. [AutoDiff][TF-1200] Adding derivatives for stdlib pow function. Apr 22, 2020
@vguerra
Copy link
Contributor Author

vguerra commented Apr 22, 2020

hi @marcrasi, I addressed your comments in 24939a5 and changed this PR to be agains master now.

@vguerra vguerra requested a review from marcrasi April 29, 2020 19:28
Copy link

@marcrasi marcrasi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now! Sorry, it took me a while to notice this.

I'll run tests and if they're green, I'll merge!

@marcrasi
Copy link

marcrasi commented May 1, 2020

@swift-ci please test

@vguerra
Copy link
Contributor Author

vguerra commented May 1, 2020

No worries @marcrasi, thanks!

@marcrasi
Copy link

marcrasi commented May 1, 2020

@swift-ci please test

@marcrasi marcrasi merged commit b433920 into swiftlang:master May 5, 2020
rxwei pushed a commit to rxwei/swift that referenced this pull request Jun 3, 2020
…iftlang#30580)

Adding JVP and VJP function derivatives for pow function
defined in stdlib.

Resolves TF-1200.
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.

3 participants