Skip to content

Conversation

jekbradbury
Copy link

Introduced in #21740. Added a test.

@jekbradbury jekbradbury requested a review from dan-zheng January 16, 2019 19:25
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.

Nice catch, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: use expectEqual when you can, and the first argument is expected to be the expected value.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall, pure GPE can't handle expectEqual (because expectEqual opaquely calls Tensor.== and isn't inlined by GPE). That's also why test/TensorFlowRuntime/tensor.swift doesn't call Tensor.== directly.

Shall we use expectEqual anyways? I feel it especially makes sense for this test file.

Copy link
Contributor

Choose a reason for hiding this comment

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

It can now, because #tfop has an ABI now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

  • We generally don't use self. unless it's required.
  • It'd be more efficient if you make it capture self.shapeTensor explicitly instead of capturing self. I'd write this as
{ [shape = shapeTensor] v in
  return v.reshaped(toShape: shape)
}

Copy link
Contributor

@dan-zheng dan-zheng Jan 16, 2019

Choose a reason for hiding this comment

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

FYI: there are definitely more places where we could use explicit captures instead of capturing self for efficiency. We should change those places at some point!

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on whether self is practically a larger tensor that the result you get from operations on self :)

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@jekbradbury jekbradbury added the tensorflow This is for "tensorflow" branch PRs. label Jan 17, 2019
@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

1 similar comment
@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei rxwei merged commit eed0f6b into swiftlang:tensorflow Jan 17, 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.

3 participants