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): Add backward method to Paddle Frontend #23661

Closed
wants to merge 3 commits into from
Closed

feat(paddle): Add backward method to Paddle Frontend #23661

wants to merge 3 commits into from

Conversation

Boghdady9
Copy link
Contributor

@Boghdady9 Boghdady9 commented Sep 15, 2023

PR Description

Add backward method to Paddle Frontend and tested it

Related Issue

Close #23407

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

Thank you for your Pull Request! We have run several checks on this pull request in order to make sure it's suitable for merging into this project. The results are listed in the following section.

Protected Branch

In order to be considered for merging, the pull request changes must not be implemented on the "main" branch. This is described in our Contributing Guide. We are closing this pull request and we would suggest that you implement your changes as described in our Contributing Guide and open a new pull request.

@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Sep 15, 2023
Comment on lines 765 to 766
def backward(self, x):
return paddle_frontend.Tensor.backward(self.x)
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to implement the backward method as this is an infinite loop

Copy link
Contributor Author

@Boghdady9 Boghdady9 Sep 16, 2023

Choose a reason for hiding this comment

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

@MahmoudAshraf97
can you explain more, I don’t get it!. how it's an infinite loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

you are calling paddle_frontend.Tensor.backward which inside your function which is also paddle_frontend.Tensor.backward, that means you are calling your function again which calls the same function over and over, an infinite recursion, please read the contribution guide in the docs and check how other functions are implemented in the same file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MahmoudAshraf97
I think when calling backward(), paddle_frontend.Tensor.backward() will be invoked one time. I was reviewing this method with someone else and it was ready to be merged but there was a minor change in the name of the test function and I made it, but I made small mistake when committed it, so I opened new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MahmoudAshraf97
please check this, it's a closed PR for the same issue: #23549

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

this code cannot and will not be merged at this stage, as I previously said there's an infinite recursion in your code that need to be solved first and the tests are not passing because of that

Copy link
Contributor

Choose a reason for hiding this comment

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

The current test does not actually test the function as it always returns None, please use a test similar to the torch frontend backward method found here:
ivy/ivy_tests/test_ivy/test_frontends/test_torch/test_tensor.py::test_torch_tensor_backward

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@Boghdady9 Boghdady9 closed this by deleting the head repository Sep 23, 2023
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.

backward
3 participants