Skip to content
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

[TOPI] Add C++ implementation of elementwise operators #1306

Merged
merged 8 commits into from
Jun 27, 2018

Conversation

nishi-t
Copy link
Contributor

@nishi-t nishi-t commented Jun 21, 2018

This PR adds c++ implementation of elementwise operators:

  • elemwise_sum
  • full
  • full_like
  • less
  • greter

@nishi-t nishi-t changed the title Add C++ implementation of elementwise operators [TOPI] Add C++ implementation of elementwise operators Jun 21, 2018
Copy link
Contributor

@PariksheetPinjari909 PariksheetPinjari909 left a comment

Choose a reason for hiding this comment

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

Redirect the python ops to c++ ops, keep the doc strings of python ops

const Tensor& rhs,
Type out_type,
std::string name = "tensor",
std::string tag = kElementWise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation

* \param rhs The input tensor
* \param out_type The type of output
*
* \return A Tensor filled with fill_value
Copy link
Contributor

Choose a reason for hiding this comment

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

add correct doc string

* \param rhs The input tensor
* \param out_type The type of output
*
* \return A Tensor filled with fill_value
Copy link
Contributor

Choose a reason for hiding this comment

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

add correct doc string

* \return A Tensor whose op member is the sum operation
*/
inline Tensor elemwise_sum(const Array<Tensor>& xs,
int num_args,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the significance of num_args?

Copy link
Contributor Author

@nishi-t nishi-t Jun 21, 2018

Choose a reason for hiding this comment

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

I add num_args as argument correspond to python implementation, but it is not used in there too. If not necessary, I'll remove it from arguments both cpp and python. Would you comment on this?
\cc @tqchen

Copy link
Member

Choose a reason for hiding this comment

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

Can be remove safely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks!

Expr fill_value,
std::string name = "tensor",
std::string tag = kElementWise) {
Expr ev = lossless_cast(dtype, fill_value);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that lossless_cast could return an undefined Expr.

Could raise an appropriate error here to user !

Expr fill_value,
std::string name = "tensor",
std::string tag = kElementWise) {
Expr ev = lossless_cast(x->dtype, fill_value);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible that lossless_cast could return an undefined Expr.

Could raise an appropriate error here to user !

topi/src/topi.cc Outdated
@@ -174,6 +174,31 @@ TVM_REGISTER_GLOBAL("topi.cast")
*rv = cast(args[0], args[1]);
});

TVM_REGISTER_GLOBAL("topi.elemwise_sum")
.set_body([](TVMArgs args, TVMRetValue *rv) {
*rv = elemwise_sum(args[0], args[1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Corresponding to above comment, number or args can be obtained inside topi by size().

@nishi-t
Copy link
Contributor Author

nishi-t commented Jun 21, 2018

@srkreddy1238 @PariksheetPinjari909 Thank you for your review. I'm acting on your comments.

@nishi-t
Copy link
Contributor Author

nishi-t commented Jun 22, 2018

@PariksheetPinjari909 @srkreddy1238 Plase review again.

* \return A Tensor filled with fill_value
*/
inline Tensor full_like(const Tensor& x,
Expr fill_value,
Copy link
Contributor

Choose a reason for hiding this comment

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

const Expr fill_value

*/
inline Tensor full(const Array<Expr>& shape,
Type dtype,
Expr fill_value,
Copy link
Contributor

Choose a reason for hiding this comment

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

const Expr fill_value

import tvm
import topi

def verify_elemwise_sum(num_args, dtype):
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove num_args here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use num_args here to generate test data for doing various element_wise tests.

np_out = np.sum(np.array(np_nd), axis=0)
np.testing.assert_allclose(out.asnumpy(), np_out, rtol=1e-5)

for device in ["llvm"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Specific reason for only llvm ?

@srkreddy1238
Copy link
Contributor

@nishi-t
other than these comments LGTM.

* \brief Creates an operation that fill a tensor with fill_value
*
* \param shape The shape of a tensor
* \param dtype The number of arguments
Copy link
Contributor

Choose a reason for hiding this comment

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

add correct doc string for dtype

* \param name The name of the operation
* \param tag The tag to mark the operation
*
* \return A Tensor filled with fill_value
Copy link
Contributor

Choose a reason for hiding this comment

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

add correct doc string

* \param name The name of the operation
* \param tag The tag to mark the operation
*
* \return A Tensor filled with fill_value
Copy link
Contributor

Choose a reason for hiding this comment

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

add correct doc string



if __name__ == "__main__":
test_elemwise_sum()
Copy link
Contributor

Choose a reason for hiding this comment

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

call test_full and test_comparator

*
* \return A Tensor filled with fill_value
*/
inline Tensor full(const Array<Expr>& shape,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the name full. It's operation is to fill the tensor with value fill_value. Shouldn't the name be 'fill'?
@tqchen python op also has the name 'full'. Can you clarify here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mxnet and numpy use full as operation name, but tensorflow use fill. maybe it's a matter of taste. I'll follow community's direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@nishi-t
Copy link
Contributor Author

nishi-t commented Jun 22, 2018

@srkreddy1238 @PariksheetPinjari909 thanks a lot. I'll fix it according to your comments.

@tqchen
Copy link
Member

tqchen commented Jun 22, 2018

please rebase against master, greater and less are added in #1321 and supported as broadcast ops

@tqchen
Copy link
Member

tqchen commented Jun 26, 2018

@nishi-t please ping the reviewers again once you have updates

@tqchen
Copy link
Member

tqchen commented Jun 26, 2018

Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

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

@nishi-t thanks.

Good to go now.

@tqchen tqchen merged commit c6feab2 into apache:master Jun 27, 2018
@tqchen
Copy link
Member

tqchen commented Jun 27, 2018

Thanks all the reviewers, this is now merged!

sergei-mironov pushed a commit to sergei-mironov/tvm that referenced this pull request Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants