Skip to content

Conversation

@bwasti
Copy link
Contributor

@bwasti bwasti commented Nov 9, 2018

Summary: This enables creation of operators with FunctionSchema and IValue

Differential Revision: D13008791

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Copy link
Collaborator

@dzhulgakov dzhulgakov left a comment

Choose a reason for hiding this comment

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

Can you add more tests, especially if the op uses the new API to change output size?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not sufficient:

  1. you assume the output is already tensor - what if it's not?
  2. this is not the api we'd be calling - instead majority of places call OutputTensor(int idx, at::IntList dims, at::TensorOptions options) below which avoids reinitialization/dtype change in-place and instead creates new tensor input

in both cases we'd need to change ivalue in-place, i.e. we need a pointer to ivalue for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense -- updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

what about non-Tensor Output() version above? at least add an option to claim that it's not a "legacy operator"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping?

Copy link
Collaborator

Choose a reason for hiding this comment

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

.get is redundant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

for a better test - don't initialize the IValue at all

Copy link
Collaborator

Choose a reason for hiding this comment

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

if IValue wasn't holding tensor - should we reset it to the tensor type? as semantically it's "assignment"

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you move it inside the if and use tensor variable - you can save one refcount bump

Copy link
Contributor

Choose a reason for hiding this comment

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

to keep the original behavior, we are changing this to tensor.raw_mutable_data(options.dtype()); see Line 48 of blob.h

Copy link
Collaborator

@dzhulgakov dzhulgakov left a comment

Choose a reason for hiding this comment

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

Please fix mine and Jerry's comments. Otherwise looks great!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ping?

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd move this wrapping inside the if still to save refcount (otherwise you still bump one when you look up tensor above (or you can get device from at::Tensor directly

Summary:
Pull Request resolved: pytorch#13789

This enables creation of operators with FunctionSchema and IValue

Reviewed By: smessmer

Differential Revision: D13008791

fbshipit-source-id: 2b558611be75d26e17c3c2d9f2e0158201b96c33
zdevito pushed a commit to zdevito/ATen that referenced this pull request Dec 6, 2018
Summary:
Pull Request resolved: pytorch/pytorch#13789

This enables creation of operators with FunctionSchema and IValue

Reviewed By: smessmer

Differential Revision: D13008791

fbshipit-source-id: 151efc88ac315f4a0ab0171a99774caaf767ef1e
@ezyang ezyang added the merged label Jun 25, 2019
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.

5 participants