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

Conform Tensor to PointwiseMultiplicative. #297

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

dan-zheng
Copy link
Member

@dan-zheng dan-zheng requested a review from rxwei June 26, 2019 07:20
@dan-zheng dan-zheng force-pushed the pointwise-multiplicative branch from ad05147 to f861b3f Compare June 26, 2019 16:01
@dan-zheng dan-zheng force-pushed the pointwise-multiplicative branch from f861b3f to bdb0913 Compare June 26, 2019 16:02
@dan-zheng
Copy link
Member Author

swiftlang/swift#25772 has been merged, so this PR can be merged once CI gets a new toolchain.

@dan-zheng dan-zheng merged commit 60f63e1 into tensorflow:master Jun 27, 2019
@dan-zheng dan-zheng deleted the pointwise-multiplicative branch June 27, 2019 03:28
@eaplatanios
Copy link
Contributor

@dan-zheng Why did you add a reciprocal but not a ./ function to this protocol? Using reciprocal and then multiplication to perform division would be about two times slower than a single ./ call.

@rxwei
Copy link
Contributor

rxwei commented Jun 28, 2019

./ should definitely be added as a protocol requirement with a default implementation.

@dan-zheng
Copy link
Member Author

@dan-zheng Why did you add a reciprocal but not a ./ function to this protocol? Using reciprocal and then multiplication to perform division would be about two times slower than a single ./ call.

Good question.

We decided not to add a division operator for mathematical reasons. My understanding:

  • Formal ring/field definitions typically involve multiplicative inverse, rather than explicit division which is not defined when dividing by zero.
  • Numeric does not define division: related discussion.

./ should definitely be added as a protocol requirement with a default implementation.

Adding a default implementation (x .* x.reciprocal) would cause derived conformances to not be triggered, leaving @eaplatanios's performance concern unaddressed.

@rxwei
Copy link
Contributor

rxwei commented Jun 28, 2019

  • Formal ring/field definitions typically involve multiplicative inverse, rather than explicit division which is not defined when dividing by zero.

The goal shouldn't be to model the exact algebraic structure, but to make these APIs practically useful. Division is by definition multiplication with multiplicative inverse, so it should be added as a default implementation at least. The reason we want to add a protocol requirement (aka. customization point) is that / and /= can almost always be implemented more efficiently than the default implementation, so we want libraries to be able to customize them.

I think this is a different problem. What PointwiseMultiplicative really models is a field, for which reason it should not be called PointwiseMultiplicative, but we just need a prototype to make optimizers work. If we are to bring this to Swift Evolution, a field protocol is supposed to be a refinement of Numeric (if Numeric did not have magnitude), and use * and / instead of .* and ./.

In short, this protocol is a prototype that shouldn't be taken super seriously. Ideally, we want to be able to use Numeric to represent rings with identity, but Numeric annoyingly has a magnitude that prevents this.

@rxwei
Copy link
Contributor

rxwei commented Jun 28, 2019

Adding a default implementation (x .* x.reciprocal) would cause derived conformances to not be triggered, leaving @eaplatanios's performance concern unaddressed.

This is indeed a pain, but it's a compiler limitation/bug. Derived conformances are supposed to use each field's protocol requirement to implement the corresponding parent type's protocol requirement. The derived conformances code path isn't able trigger this automatically yet AFAIK.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants