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

add inplace pointwise operators #224

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iclementine
Copy link
Collaborator

@iclementine iclementine commented Sep 24, 2024

PR Category

Operator

Type of Change

New Feature

Description

Add inplace pointwise operators.

Issue

Progress

  • Change is properly reviewed (1 reviewer required, 2 recommended).
  • Change is responded to an issue.
  • Change is fully covered by a UT.

Performance

tongxin
tongxin previously approved these changes Sep 25, 2024
Copy link
Contributor

@tongxin tongxin left a comment

Choose a reason for hiding this comment

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

LGTM

@iclementine
Copy link
Collaborator Author

I have to solve the problem at when the output has in-appropriate dtype or shape it does not raise error.

@iclementine iclementine marked this pull request as draft September 25, 2024 07:58
@Bowen12992 Bowen12992 marked this pull request as ready for review September 25, 2024 09:14
@@ -37,4 +37,16 @@ def add(A, B, *, alpha=1):
elif isinstance(B, torch.Tensor):
return add_func_scalar_tensor(A, B, alpha)
else:
return A + B * alpha
return torch.tensor(A + B * alpha)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

torch returns a tensor for that case.

@@ -32,6 +32,21 @@ def test_accuracy_abs(shape, dtype):
gems_assert_equal(res_out, ref_out)


@pytest.mark.inplace
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is inplace mark used for? can we merge abs_ test into abs test?

Bowen12992
Bowen12992 previously approved these changes Sep 25, 2024
Copy link
Collaborator

@Bowen12992 Bowen12992 left a comment

Choose a reason for hiding this comment

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

LGTM, is it necessary to add benchmark for those inplace ops?

@tongxin
Copy link
Contributor

tongxin commented Sep 26, 2024

LGTM, is it necessary to add benchmark for those inplace ops?

It'll add a lot of redundancy if we include all the variants. Shall we do that later?

@FatJhon FatJhon dismissed stale reviews from Bowen12992 and tongxin via 037dd0c December 23, 2024 12:34
@tongxin
Copy link
Contributor

tongxin commented Dec 24, 2024

It's probably a good time to update this PR?

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.

3 participants