Skip to content

Add 2 ops zeros and zeros_like. #251

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

Merged
merged 9 commits into from
Dec 15, 2022
Merged

Conversation

fatcat-z
Copy link
Contributor

Try to implement 2 ops in aten lib functions: zeros and zeros_like.

@justinchuby justinchuby self-requested a review December 13, 2022 16:08
@justinchuby justinchuby added the module: torchlib Related to the torch/aten function lib in development label Dec 13, 2022
@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #251 (c34c5c2) into main (2512120) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
+ Coverage   72.16%   72.20%   +0.04%     
==========================================
  Files          93       93              
  Lines        8916     8923       +7     
==========================================
+ Hits         6434     6443       +9     
+ Misses       2482     2480       -2     
Impacted Files Coverage Δ
...t/function_libs/torch_aten/ops_correctness_test.py 95.93% <ø> (ø)
onnxscript/function_libs/torch_aten/ops/core.py 53.87% <100.00%> (+0.41%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zero = op.Constant(value=0) # type: ignore[arg-type]
else:
zero = op.Cast(0, to=dtype) # type: ignore[arg-type]
return op.ConstantOfShape(size, zero)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ConstantOfShape takes an attribute as the second argument. You may need to use expand

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
Contributor Author

Choose a reason for hiding this comment

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

ConstantOfShape takes an attribute as the second argument. You may need to use expand

Got it, has updated to use a constant of tensor instead.

I guess ConstantOfShape is just like a constant, while Expand may still need to do some computation to broadcast.
Thoughts?

Copy link
Collaborator

@justinchuby justinchuby Dec 14, 2022

Choose a reason for hiding this comment

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

As long as it runs I am for anything that’s more simple. Were you able to run the tests? Expand was what Rama suggested when I initially used ConstantOfShape, because the zero here is not compile time constant.

from onnxscript import BOOL, INT64
from onnxscript.onnx_opset import opset18 as op
from onnxscript.onnx_types import TensorType

const_zero_int = helper.make_tensor("zero_tensor", TensorProto.INT64, [1], [0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note: for anything that is a global constant, prefer UPPER_CASE. If private, prefix with _.

https://google.github.io/styleguide/pyguide.html#316-naming

@@ -17,10 +17,12 @@

from typing import Any, Optional, Sequence

from onnx import TensorProto, helper
Copy link
Collaborator

@justinchuby justinchuby Dec 15, 2022

Choose a reason for hiding this comment

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

@fatcat-z fatcat-z merged commit 8bd6dbd into microsoft:main Dec 15, 2022
@fatcat-z fatcat-z deleted the new_aten_ops branch December 15, 2022 11:54
This was referenced Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: torchlib Related to the torch/aten function lib in development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants