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

Make Element requirement in TensorArrayProtocol.init a conformance. #171

Merged
merged 3 commits into from
Jun 3, 2019

Conversation

bgogul
Copy link
Contributor

@bgogul bgogul commented Jun 3, 2019

@bgogul bgogul requested a review from rxwei June 3, 2019 21:09
@rxwei
Copy link
Contributor

rxwei commented Jun 3, 2019

Do you ever need to call init(_handles:) on an array of type-erased _AnyTensorHandles? If so, this change would make that impossible.

@@ -35,7 +35,7 @@ public protocol TensorArrayProtocol {
var _tensorHandles: [_AnyTensorHandle] { get }

init(_owning tensorHandles: UnsafePointer<CTensorHandle>?, count: Int)
init<C: RandomAccessCollection>(_handles: C) where C.Element == _AnyTensorHandle
init<C: RandomAccessCollection>(_handles: C) where C.Element : _AnyTensorHandle
Copy link
Contributor

Choose a reason for hiding this comment

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

The Google Swift Style Guide recommends no whitespaces before colons in a conformance constraint.

Suggested change
init<C: RandomAccessCollection>(_handles: C) where C.Element : _AnyTensorHandle
init<C: RandomAccessCollection>(_handles: C) where C.Element: _AnyTensorHandle

@bgogul
Copy link
Contributor Author

bgogul commented Jun 3, 2019

Do you ever need to call init(_handles:) on an array of type-erased _AnyTensorHandles? If so, this change would make that impossible.

I have not needed it for the implementation of LazyTensor, but there is one use case in the test. That is why I made the following change:

+copy(of: TensorHandle<T>) -> TFETensorHandle
-copy(of: TensorHandle<T>) -> _AnyTensorHandle

Without the changes in this PR, I was not able to initialize a TensorGroup with an array of specific type, e.g., [TFETensorHandle] or [LazyTensor]. Is there a better way to achieve this?

@rxwei
Copy link
Contributor

rxwei commented Jun 3, 2019

A [TFETensorHandle] should be directly convertible to a [_AnyTensorHandle] because it's a subtype.

@bgogul
Copy link
Contributor Author

bgogul commented Jun 3, 2019

A [TFETensorHandle] should be directly convertible to a [_AnyTensorHandle] because it's a subtype.

Ah, that is right. The problem that I faced was with using an ArraySlice<TFETensorHandle> as argument to init(_handles:_).

@rxwei
Copy link
Contributor

rxwei commented Jun 3, 2019

I see. Yeah, variance only works with a few stdlib types in Swift such as Array.

@bgogul bgogul merged commit 802af3e into tensorflow:master Jun 3, 2019
@bgogul bgogul deleted the element_conf branch June 3, 2019 22:07
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