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: add increment method to paddle frontend #26331

Conversation

muhammadhammad-tech
Copy link
Contributor

@muhammadhammad-tech muhammadhammad-tech commented Sep 29, 2023

Fixes #26215

PR Description

Implemented the increment method in both math.py, test_math.py, tensor.py, and test_tensor.py.

Related Issue

Close #26215

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!

@ivy-leaves ivy-leaves added Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist labels Sep 29, 2023
@muhammadhammad-tech
Copy link
Contributor Author

I made changes in the following files tensor.py, test_tensor.py,math.py,test_math.py

@muhammadhammad-tech
Copy link
Contributor Author

@juliagsy kindly can you review the PR

@muhammadhammad-tech muhammadhammad-tech changed the title increment feat: add increment method to paddle frontend Sep 30, 2023
Copy link
Contributor

@juliagsy juliagsy left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for contributing! The increment method is relying on its functional version and it is currently being worked on in another issue and PR, I'll check back soonest possible when that it available because I wouldn't be able to test and make sure they are working correctly and as expected otherwise. Meanwhile, could you please remove all changes in the docs/demos folder as well as the functional version? Thanks!

ivy/functional/frontends/paddle/math.py Outdated Show resolved Hide resolved
Copy link
Contributor

@KareemMAX KareemMAX left a comment

Choose a reason for hiding this comment

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

Hi, seems like you have changed docs/demos which you might have done by mistake. Can you revert that change because we can't merge with that change in place?

@muhammadhammad-tech
Copy link
Contributor Author

thank you @juliagsy and @KareemMAX , i dont know how the docs/demos file got changed I am working on it. and kindly would you tell me should I remove my increment implementation from every file and make it once the increment function is done ?

@muhammadhammad-tech
Copy link
Contributor Author

kindly can you guys help me on this docs/demos i am unable to revert it back i reverted all my changes upto merge 'main' commit, thank you

@muhammadhammad-tech
Copy link
Contributor Author

@juliagsy hopefully resolved the conflicts can you look it again guide for further changes thank you

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Thank you for this PR, here is the CI results:


This pull request does not result in any additional test failures. Congratulations!

@juliagsy
Copy link
Contributor

juliagsy commented Oct 5, 2023

could you please try cd docs/demos then git checkout 90869f9 then commit to the branch/pr to see if the demos folder will revert back to the commit in master?

The other looks good to me as of now, I'll test them out when the functional version is ready, will also attach that PR here so that we can check the status easily. #21369

Thanks!

@muhammadhammad-tech
Copy link
Contributor Author

i followed these steps every time this PR had the conflict with the docs/demos file each time it got me demos file that i commit then the conflict ended but now i am doing nothing is happening, i tried your method but also doesnt work tbh i am confused
image_2023-10-06_023101258

@muhammadhammad-tech
Copy link
Contributor Author

image

@muhammadhammad-tech
Copy link
Contributor Author

should I use git rebase ?? will it be help full need help @juliagsy
thank you

@muhammadhammad-tech
Copy link
Contributor Author

image

@muhammadhammad-tech
Copy link
Contributor Author

image

@juliagsy
Copy link
Contributor

juliagsy commented Oct 9, 2023

@muhammadhammad-tech Hey! apologies for the delay, hmm could you try a rebase within docs/demos? I haven't com across this so far, maybe @KareemMAX : have you come across this in other PRs before?) thanks!

@KareemMAX
Copy link
Contributor

I guess merge is enough, haven't used rebase before on submodules

@muhammadhammad-tech
Copy link
Contributor Author

@KareemMAX @juliagsy it fine or should ? what should i do ? thank you

@muhammadhammad-tech
Copy link
Contributor Author

it showing no conflict at this moment.

@juliagsy
Copy link
Contributor

yeah, then I guess it's fine for now, since from the PR diff the commits are from internally instead of something you changed or committed in extra. for now let's see if the dependent PR is merged within the coming week, and we'll look into merging yours soon after, thanks again!

@juliagsy
Copy link
Contributor

Hey! it seems the contributor of another PR is inactive and closed their PR. Apologies for the hassle but you are now safe to add the functional version of increment in your PR for the method version to work. Let me know if you have any question, thanks!

@muhammadhammad-tech
Copy link
Contributor Author

hey @juliagsy can you help i don't know what happened

@muhammadhammad-tech
Copy link
Contributor Author

it got closed automatically i cant even reopen it can you help should i do it from scratch.. thank you need your help @juliagsy

@muhammadhammad-tech
Copy link
Contributor Author

reopening it

@juliagsy
Copy link
Contributor

juliagsy commented Oct 16, 2023

Hey! I haven't encountered this before too but I'm guessing its something to do with the force push, but you should be able to open a new PR through the branch in your fork, hope this is helpful!

edit: unfortunately I'm unable to reopen it too

@muhammadhammad-tech
Copy link
Contributor Author

okay thank you for the response

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 PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

increment
4 participants