Skip to content

[AutoDiff upstream] add @noDerivative to AnyFunctionType params #28278

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
Dec 19, 2019

Conversation

marcrasi
Copy link

@marcrasi marcrasi commented Nov 15, 2019

Adds the @noDerivative attribute to AnyFunctionType parameters.

This attribute is used to determine which parameters a differentiable function type is differentiable with respect to, e.g. @differentiable (Float, @noDerivative Float) -> Float is only differentiable with respect to its first parameter.

Also adds serialization for the attribute.

This resolves TF-870.

@marcrasi marcrasi changed the title add @nondiff to AnyFunctionType params [AutoDiff upstreaming] add @nondiff to AnyFunctionType params Nov 15, 2019
@lattner
Copy link
Contributor

lattner commented Nov 15, 2019

The name @nondiff is a bit weird. Was @nondifferentiable considered?

@lattner
Copy link
Contributor

lattner commented Nov 15, 2019

Well, that isn't right either, because it is possible differentiable but you don't want it in this case - better names still desired :)

Copy link
Contributor

@lattner lattner left a comment

Choose a reason for hiding this comment

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

this LGTM, but the name @nondiff isn't great. For the time being, I'd suggest going with @nondifferentiable to align with the name inside of the compiler unless there is a better suggestion.

@marcrasi
Copy link
Author

marcrasi commented Nov 15, 2019

I discussed the naming with @dan-zheng, and we think that @noDerivative would be a pretty good name, because the meaning is very similar to the @noDerivative attribute on struct fields. For example, if you wanted to explode a struct in a function type, keeping @noDerivative on the corresponding fields is exactly what you need to do:

struct Foo {
  let a: Float
  @noDerivative let b: Float
}

let f: @differentiable (Foo) -> Float
let g: @differentiable (Float, @noDerivative Float) -> Float

It's already named @nondiff in the tensorflow branch, and various client code uses it, so I'm going to have to do a deprecate-then-remove thingy, but this shouldn't be too hard.

cc @rxwei for opinions.

I'll wait a bit to see if anyone disagrees before I make the code changes.

@rxwei
Copy link
Contributor

rxwei commented Nov 15, 2019

I think we should use @noDerivative.

@marcrasi
Copy link
Author

I have renamed this to @noDerivative. I will run tests but wait until Monday to merge, to avoid causing any postsubmit CI failures over the weekend.

@marcrasi
Copy link
Author

@swift-ci please test

1 similar comment
@marcrasi
Copy link
Author

@swift-ci please test

@marcrasi marcrasi changed the title [AutoDiff upstreaming] add @nondiff to AnyFunctionType params [AutoDiff upstreaming] add @noDerivative to AnyFunctionType params Nov 16, 2019
@lattner
Copy link
Contributor

lattner commented Nov 16, 2019

+1 for @noDerivative !

@rxwei rxwei changed the title [AutoDiff upstreaming] add @noDerivative to AnyFunctionType params [AutoDiff upstream] add @noDerivative to AnyFunctionType params Nov 16, 2019
rxwei added a commit that referenced this pull request Nov 17, 2019
We agreed to move to rename `@nondiff` to `@noDerivative` (in line with the `@noDerivative` declaration attribute) in #28278.
@marcrasi
Copy link
Author

I have resolved merge conflicts, and I plan to do a test and merge soon.

@marcrasi
Copy link
Author

@swift-ci please test and merge

1 similar comment
@marcrasi
Copy link
Author

@swift-ci please test and merge

@swift-ci swift-ci merged commit ec0a2ca into swiftlang:master Dec 19, 2019
dan-zheng pushed a commit that referenced this pull request Dec 20, 2019
Cherry-picks #28278 to `tensorflow` branch.
`@nondiff` exists with a deprecation warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants