Skip to content

Conversation

@vguerra
Copy link
Contributor

@vguerra vguerra commented Oct 30, 2019

The following math functions are now differentiable:

  • remainder
  • fmod
  • ceil
  • floor
  • round
  • trunc

Resolves TF-812

The following math functions are now differentiable:

* `remainder`
* `fmod`
* `ceil`
* `floor`
* `round`
* `trunc`

Resolves [TF-812](https://bugs.swift.org/projects/TF/issues/TF-812)
@saeta saeta requested review from dan-zheng and marcrasi October 30, 2019 15:22
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.

Rather than using @differentiable(..., vjp: ...) on the original tgmath functions, could you please use @differentiating on the derivative functions?

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Oct 30, 2019
@vguerra
Copy link
Contributor Author

vguerra commented Oct 30, 2019

Rather than using @differentiable(..., vjp: ...) on the original tgmath functions, could you please use @differentiating on the derivative functions?

Sure, will do that. Commit coming soon.

@vguerra
Copy link
Contributor Author

vguerra commented Oct 31, 2019

hello @dan-zheng ... I tried to define the derivates using the @differentiating attribute but unfortunately I get a compiler crash... you can see the details of the crash in this gist. Does this ring a bell? Is it an issue you might have seen before?

@dan-zheng
Copy link
Contributor

hello @dan-zheng ... I tried to define the derivates using the @differentiating attribute but unfortunately I get a compiler crash... you can see the details of the crash in this gist. Does this ring a bell? Is it an issue you might have seen before?

Thanks for sharing! This issue looks like TF-429: the differentiation transform does not yet support -enable-library-evolution (previously called -enable-resilience). The standard library is currently built with -enable-library-evolution.

I wouldn't expected @differentiable vs @differentiating attribute to have an effect on TF-429. I wonder if the crash occurs with @differentiable attribute? I'll trigger CI to find out.

@dan-zheng
Copy link
Contributor

@swift-ci Please test tensorflow

@vguerra
Copy link
Contributor Author

vguerra commented Nov 1, 2019

This PR, as it is, compiles fine in my machine. Let’s see what’s the result of the CI.

@dan-zheng
Copy link
Contributor

CI passes.

@vguerra: would you mind starting another PR using @differentiating attributes to register derivatives? If CI fails for @differentiating attributes, we can minimize the issue and see if it's relevant to TF-429.

@vguerra
Copy link
Contributor Author

vguerra commented Nov 5, 2019

hi @dan-zheng .. sure, I can do that. I'll start a new PR.

@vguerra
Copy link
Contributor Author

vguerra commented Nov 6, 2019

I've started a new PR #28108

@dan-zheng
Copy link
Contributor

Let's close this in favor of #28108, since it uses @differentiating to register derivatives instead of @differentiable.

@dan-zheng dan-zheng closed this Nov 8, 2019
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