Support various dimensions in CosineSimilarity #157

mejai1206 · 2023-10-05T02:09:05Z

🙏 Describe the pull request

CosineSimiliarity에 3차원 보다 작은 입력을 지원합니다.

✅ Checklist

Code follows the project's coding conventions and style.
Tests have been added or updated to cover the changes.
Documentation has been updated, if necessary.

daemyung

제목이 52자가 넘는데 제목을 좀 더 명료하게 작성해주세요. 표현하고 싶은 내용은 본문으로 옮기면 됩니다.

trident/operation/cosine_similarity.py

daemyung · 2023-10-05T02:47:49Z

tests/test_cosine_similarity.py

    factory_kwargs = {"device": device}
+    x_dims = [(z_size, y_size, x_size), (y_size, x_size), (x_size,)]


@pytest.mark.parametrize가 tuple로 정의되는게 더 좋을 것 같아요.

아래와 같은 형태로 수정하였습니다.

@pytest.mark.parametrize( "x_dim", [(1431, 500, 200), (21, 6400), (86,)], )

trident/operation/cosine_similarity.py

mejai1206 · 2023-10-05T04:50:30Z

커밋/PR 제목을 Add input dimension of CosineSimiliarity로 수정하고
상세 내용은 본문으로 수정하였습니다.

daemyung

리뷰를 하기 전에 용어에 대한 정리를 해야할 것 같습니다.

dim은 dimension의 약자로 차원을 나타내는 용어입니다. 그래서 tensor.dim()을 하면 텐서가 가지고 있는 차원이 반환됩니다. 그리고 1차원, 2차원, 3차원을 표시할때도 dim이나 axis를 사용해서 표현합니다.

shape은 텐서의 모양을 의미합니다. 예를 들어 3차원 텐서의 모양은 [16, 32, 64]와 같습니다.

정리하자면 dim은 텐서의 차원이나 텐서에서 특정 차원을 지칭하기 위해 사용되는 단어입니다. 그리고 shape은 텐서의 모양을 표현하는 단어이며 dim의 크기를 가집니다.

test_cosine_similarity.py의 수정사항을 보면 x_dim이란 변수 이름으로 (1431, 500, 200), (21, 6400), (86,)가 정의되고 있습니다. x_dim 이름이 적절할까요? 아마 적절하지 않다고 생각할 것입니다. x_shape이나 input_shape이 더 적절하겠죠? x_dim은 1, 2, 3과 같은 숫자가 들어와야합니다.

다시 한번 작성한 코드를 살펴보시고 수정해 보시겠어요?

mejai1206 · 2023-10-05T06:47:39Z

dim 과 shape에 대해서 변수명을 아래와 같이 정리하였습니다.

x_dim --> x_shape
output_dim --> output_shape

daemyung

지금의 커밋 제목은 변경 사항을 제대로 전달하지 못하는 것 같습니다. Support various dimensions in CosineSimilarity는 어떠세요?

daemyung · 2023-10-05T06:53:27Z

tests/test_cosine_similarity.py

-        torch.nn.functional.cosine_similarity(x1, x2, dim=dim),
-        trident.function.cosine_similarity(x1, x2, dim=dim),
-    )
+    for dim in range(len(x_shape)):


모든 dim을 테스트할 필요가 있나요? 테스트 할 dim을 파라미터로 넘기면 더 좋을 것 같습니다.

특정 dim만 파라미터로 지정해서 테스트하도록 수정하였습니다.

trident/operation/cosine_similarity.py

mejai1206 · 2023-10-05T07:19:03Z

Support various dimensions in CosineSimilarity 커밋명 수정하였습니다.

daemyung · 2023-10-05T08:17:24Z

tests/test_cosine_similarity.py

+                return (x_shape[0], x_shape[1])
+
+    output_shape = get_output_shape(x_shape, dim)
+    grad_output = torch.randn(output_shape, **factory_kwargs) if output_shape != "scalar" else None


grad_output이 None인 경우가 정상적인 경우인가요?

backward()에 스칼라를 넣으면 다음과 같은 오류가 발생하여 None 처리를 하였습니다.

TypeError: gradients can be either Tensors or None, but got str

1크기의 텐서를 만들어서 넣어야할 것 같은데요?

아래와 같이 1크기의 텐서가 들어가도록 수정하였습니다.

def get_output_shape(x_shape, dim): if len(x_shape) == 1: return () elif len(x_shape) == 2: return x_shape[1] if dim == 0 else x_shape[0] else: if dim == 0: return (x_shape[1], x_shape[2]) elif dim == 1: return (x_shape[0], x_shape[2]) else: return (x_shape[0], x_shape[1]) grad_output = torch.randn(get_output_shape(x_shape, dim), **factory_kwargs)

daemyung

LGTM

daemyung · 2023-10-05T08:51:36Z

tests/test_cosine_similarity.py

+                return (x_shape[0], x_shape[1])
+
+    output_shape = get_output_shape(x_shape, dim)
+    grad_output = torch.randn(output_shape, **factory_kwargs) if output_shape != "scalar" else None


1크기의 텐서를 만들어서 넣어야할 것 같은데요?

kakao-steve-ai

LGTM

daemyung

LGTM

mejai1206 requested a review from kakao-steve-ai October 5, 2023 02:09

mejai1206 requested a review from daemyung as a code owner October 5, 2023 02:09

daemyung suggested changes Oct 5, 2023

View reviewed changes

mejai1206 force-pushed the support_cosine_similiarity_dim branch from 3d3ad00 to 3dd3b38 Compare October 5, 2023 04:47

mejai1206 changed the title ~~Supports dimensions smaller than 3 on CosineSimiliarity~~ Add input dimension of CosineSimiliarity Oct 5, 2023

mejai1206 requested a review from daemyung October 5, 2023 04:50

daemyung suggested changes Oct 5, 2023

View reviewed changes

mejai1206 force-pushed the support_cosine_similiarity_dim branch from 3dd3b38 to d34dbc6 Compare October 5, 2023 06:44

mejai1206 requested a review from daemyung October 5, 2023 06:49

mejai1206 force-pushed the support_cosine_similiarity_dim branch from d34dbc6 to d7e13fb Compare October 5, 2023 06:50

daemyung suggested changes Oct 5, 2023

View reviewed changes

mejai1206 force-pushed the support_cosine_similiarity_dim branch from d7e13fb to 2e39eea Compare October 5, 2023 07:17

mejai1206 changed the title ~~Add input dimension of CosineSimiliarity~~ Support various dimensions in CosineSimilarity Oct 5, 2023

mejai1206 requested a review from daemyung October 5, 2023 07:19

mejai1206 self-assigned this Oct 5, 2023

daemyung reviewed Oct 5, 2023

View reviewed changes

daemyung approved these changes Oct 5, 2023

View reviewed changes

daemyung suggested changes Oct 5, 2023

View reviewed changes

kakao-steve-ai approved these changes Oct 5, 2023

View reviewed changes

Support various dimensions in CosineSimilarity

a0e0cfe

mejai1206 force-pushed the support_cosine_similiarity_dim branch from 2e39eea to a0e0cfe Compare October 5, 2023 10:31

daemyung approved these changes Oct 5, 2023

View reviewed changes

daemyung merged commit 9b77767 into main Oct 5, 2023
1 check passed

daemyung deleted the support_cosine_similiarity_dim branch October 5, 2023 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support various dimensions in CosineSimilarity #157

Support various dimensions in CosineSimilarity #157

mejai1206 commented Oct 5, 2023

daemyung left a comment •

edited

Loading

daemyung Oct 5, 2023

mejai1206 Oct 5, 2023

mejai1206 commented Oct 5, 2023 •

edited

Loading

daemyung left a comment

mejai1206 commented Oct 5, 2023 •

edited

Loading

daemyung left a comment

daemyung Oct 5, 2023

mejai1206 Oct 5, 2023 •

edited

Loading

mejai1206 commented Oct 5, 2023

daemyung Oct 5, 2023

mejai1206 Oct 5, 2023

daemyung Oct 5, 2023

mejai1206 Oct 5, 2023

daemyung left a comment

daemyung Oct 5, 2023

kakao-steve-ai left a comment

daemyung left a comment

		factory_kwargs = {"device": device}
		x_dims = [(z_size, y_size, x_size), (y_size, x_size), (x_size,)]

Support various dimensions in CosineSimilarity #157

Support various dimensions in CosineSimilarity #157

Conversation

mejai1206 commented Oct 5, 2023

🙏 Describe the pull request

✅ Checklist

daemyung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejai1206 commented Oct 5, 2023 • edited Loading

daemyung left a comment

Choose a reason for hiding this comment

mejai1206 commented Oct 5, 2023 • edited Loading

daemyung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejai1206 Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

mejai1206 commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daemyung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakao-steve-ai left a comment

Choose a reason for hiding this comment

daemyung left a comment

Choose a reason for hiding this comment

daemyung left a comment •

edited

Loading

mejai1206 commented Oct 5, 2023 •

edited

Loading

mejai1206 commented Oct 5, 2023 •

edited

Loading

mejai1206 Oct 5, 2023 •

edited

Loading