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: Added inplace Remainder in paddle frontend #22962

Closed
wants to merge 33 commits into from

Conversation

ArsalanAli915
Copy link
Contributor

@ArsalanAli915 ArsalanAli915 commented Sep 2, 2023

Close #22961

PR Description

Related Issue

Close #

Checklist

  • Did you add a function?
  • Did you add the tests?
  • Did you follow the steps we provided?

Socials:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2023

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Sep 2, 2023
@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end Please review the PR. It passing all the test on my side.
r1

@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end waiting for review.

@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end could you please take a look at PR? it's been 1 week now.

Copy link
Contributor

@hello-fri-end hello-fri-end left a comment

Choose a reason for hiding this comment

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

Hey @ArsalanAli915, thanks for working on this issue and apologies for the delayed response. Just a few minor comments and we are good to merge:

  1. You should only add paddle.remainder_ function as part of this PR..
  2. The corresponding test should be implemented in ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_paddle_math.py.

Please make the required changes and re-request a review. Happy to clarify any doubts :)

ivy/functional/frontends/paddle/tensor/math.py Outdated Show resolved Hide resolved
ivy/functional/frontends/paddle/tensor/math.py Outdated Show resolved Hide resolved
@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end Thanks for providing the kind review in the PR. As for the path ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_paddle_math.py is concerned, I think it is changed now as ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_math.py because I can find it in the repo. Please do let know if I can do anything else.

@ArsalanAli915
Copy link
Contributor Author

ArsalanAli915 commented Sep 10, 2023

image
@hello-fri-end
These are files at ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/ path.

Copy link
Contributor

@hello-fri-end hello-fri-end left a comment

Choose a reason for hiding this comment

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

Hey @ArsalanAli915. You are right, the file seems to be renamed to ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_math.py. Let's resolve the comments from the previous review then. Happy to review again once you are done. Thanks!

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!

@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end Thanks for helping me with a double check. I have resolved previous reviews. But still, if u think there is something missing please let me know. I am always there to act. thanks.

@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end Also I have tried to resolve lint in CI but It is passing on locally but IDK why it's not passing on CI. I think this is something related to the main repo. thansk.

@hello-fri-end
Copy link
Contributor

hello-fri-end commented Sep 16, 2023

ivy-gardener
✅ Ivy gardener has formatted your code.
If changes are requested, don't forget to pull your fork.

@hello-fri-end hello-fri-end changed the title Added inplace Remainder in paddle frontend feat: Added inplace Remainder in paddle frontend Sep 21, 2023
Copy link
Contributor

@hello-fri-end hello-fri-end left a comment

Choose a reason for hiding this comment

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

Hey @ArsalanAli915 , as mentioned last time, could you please undo all the unrelated changes from ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_math.py? You should only add the test for the in-place remainder function and not modify anything else. Happy to review once again after you have resoved this.

@ArsalanAli915
Copy link
Contributor Author

@hello-fri-end Thank you very much for giving my PR so much consideration. I really appreciate it. Actully due to change in files again bu ivy I have created new PR. where every thing ready to merge. This is the link of PR #25779 solved this issue. Please do look and let me know anything needed to be changed. thanks.

@hello-fri-end
Copy link
Contributor

@hello-fri-end Thank you very much for giving my PR so much consideration. I really appreciate it. Actully due to change in files again bu ivy I have created new PR. where every thing ready to merge. This is the link of PR #25779 solved this issue. Please do look and let me know anything needed to be changed. thanks.

Looks like it has already been merged. Thanks for the contribution 🥳

@ArsalanAli915 ArsalanAli915 deleted the remain_ branch September 27, 2023 08:18
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.

remainder_
4 participants