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

feat: Paddle Frontend implemented special method __mod__ #28245

Merged
merged 3 commits into from
Feb 25, 2024

Conversation

druvdub
Copy link
Contributor

@druvdub druvdub commented Feb 11, 2024

PR Description

Related Issue

Closes #27318

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you run your tests and are your tests passing?
  • Did pre-commit not fail on any check?
  • Did you follow the steps we provided?

Socials

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

PR Compliance Checks Passed!

@druvdub druvdub changed the title implement __mod__ feat: Paddle Frontend implemented special method __mod__ Feb 11, 2024
@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Feb 11, 2024
@@ -264,6 +264,10 @@ def __int__(self):
def __long__(self):
return int(self._ivy_array)

@with_supported_dtypes({"2.6.0 and below": ("int32", "int64")}, "paddle")
def __mod__(self, y, /, name=None):
Copy link
Contributor Author

@druvdub druvdub Feb 11, 2024

Choose a reason for hiding this comment

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

A Question for the reviewer: a doubt with the implementation of mod method (within this same file)
hence the implementation of this method is same as well, however,

Note:
paddle.remainder supports broadcasting. If you want know more about broadcasting, please refer to Introduction to Tensor_ .
.. _Introduction to Tensor: ../../guides/beginner/tensor_en.html#chapter5-broadcasting-of-tensor
And mod, floor_mod are all functions with the same name

            >>> import paddle

            >>> x = paddle.to_tensor([2, 3, 8, 7])
            >>> y = paddle.to_tensor([1, 5, 3, 3])
            >>> z = paddle.remainder(x, y)
            >>> print(z)
            Tensor(shape=[4], dtype=int64, place=Place(cpu), stop_gradient=True,
            [0, 3, 2, 1])

            >>> z = paddle.floor_mod(x, y)
            >>> print(z)
            Tensor(shape=[4], dtype=int64, place=Place(cpu), stop_gradient=True,
            [0, 3, 2, 1])

            >>> z = paddle.mod(x, y)
            >>> print(z)
            Tensor(shape=[4], dtype=int64, place=Place(cpu), stop_gradient=True,
            [0, 3, 2, 1])

the code under https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/tensor/math.py

seems to suggest something else.

There is also the fact that there is a mod alias defined for the remainder method in https://github.com/unifyai/ivy/blob/e6e72f97c94e2518dc16e8bd63d49ec322579a97/ivy/functional/frontends/paddle/math.py#L707 but there is an available definition for floor_mod which results in an inconsistency of definitions and usage

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @druvdub,
I couldn't understand your doubt, could you please clerify it.

Copy link
Contributor Author

@druvdub druvdub Feb 11, 2024

Choose a reason for hiding this comment

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

Hi @NiteshK84, sorry I was just trying to understand if the implementation was right, mainly because in paddlepaddle all methods remainder, floor_mod and mod are supposed to have the same behaviour but we have defined it differently for all 3 here.

It should be fine if this implementation is fine. It is more of a personal doubt.

@druvdub
Copy link
Contributor Author

druvdub commented Feb 11, 2024

Validation:
image
Tests pass as of now. Might need updating once the question to reviewer has been answered.

@NiteshK84
Copy link
Contributor

other than that, PR looks good to be merged.

@NiteshK84 NiteshK84 merged commit 13670b7 into Transpile-AI:main Feb 25, 2024
265 of 277 checks passed
@druvdub druvdub deleted the implement-__mod__ branch February 25, 2024 23:57
Kacper-W-Kozdon pushed a commit to Kacper-W-Kozdon/ivy that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__mod__
3 participants