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 Bucketize):adding test function for bucketize #26665

Closed
wants to merge 17 commits into from

Conversation

boopathiviky
Copy link

@boopathiviky boopathiviky commented Oct 5, 2023

PR Description

Related Issue

Close #

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.

Issue Reference

In order to be considered for merging, the pull request description must refer to a specific issue number. This is described in our contributing guide and our PR template.
This check is looking for a phrase similar to: "Fixes #XYZ" or "Resolves #XYZ" where XYZ is the issue number that this PR is meant to address.

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.

Conventional Commit PR Title

In order to be considered for merging, the pull request title must match the specification in conventional commits. You can edit the title in order for this check to pass.
Most often, our PR titles are something like one of these:

  • docs: correct typo in README
  • feat: implement dark mode"
  • fix: correct remove button behavior

Linting Errors

  • Found type "null", must be one of "feat","fix","docs","style","refactor","perf","test","build","ci","chore","revert"
  • No subject found

@boopathiviky boopathiviky mentioned this pull request Oct 5, 2023
@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!

@boopathiviky boopathiviky changed the title Bucketize test function Paddle Bucketize Oct 7, 2023
Copy link
Author

@boopathiviky boopathiviky left a comment

Choose a reason for hiding this comment

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

implemented all for bucketize have look.

@boopathiviky boopathiviky changed the title Paddle Bucketize feat(paddle Bucketize):adding test function for bucketize Oct 7, 2023
@boopathiviky boopathiviky mentioned this pull request Oct 7, 2023
@ivy-leaves ivy-leaves added the Paddle Frontend Developing the Paddle Frontend, checklist triggered by commenting add_frontend_checklist label Oct 9, 2023
@ivy-seed ivy-seed assigned nassimberrada and unassigned DecFox Oct 14, 2023
Copy link
Contributor

@nassimberrada nassimberrada left a comment

Choose a reason for hiding this comment

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

Hi @boopathiviky, thank you for contributing! Overall looks good, just left a couple minor comments. Besides that, could you revert the changes you PR make to the misc.xml and _template__of_py_test.xml files ? Also, please make sure the tests pass locally as the intelligent tests are currently unstable and in the process of being fixed. Thanks!

{"2.5.1 and below": ("float32", "float64", "int32", "int64")},
"paddle",
)
def bucketize(self, y, right=False, name=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to keep consistent signatures so I guess using sorted_sequence instead of y would make more sense, same for addingout_int32

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments. All the changes are made and please have look.

test_flags,
):
dtype, input = dtype_and_values
input[0] = np.sort(input[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this sorting needed ?

Copy link
Author

Choose a reason for hiding this comment

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

yes, this not used so for by mistake this could happen. i made changes pls have a look.

This reverts commit 43023d6.
tesnor.py and test_search.py
@nassimberrada
Copy link
Contributor

Looks good, as a final point, could you:
(a) Actually remove the instance method implementation in ivy/functional/frontends/paddle/tensor/tensor.py, this would help separate the two implementations in distinct PRs and better keep track of changes and issues given each implementation should have its own tests
(b) Link your PR to the appropriate issue linked to the paddle functions To-Do list
We should be able to get your PR merged once that is done, happy to answer any question. Thanks!

boopathiviky and others added 2 commits October 27, 2023 20:12
Bucketize instance is removed in tensor.py file
@boopathiviky
Copy link
Author

All changes have made as per comment. kindly have look.

@boopathiviky boopathiviky mentioned this pull request Oct 27, 2023
@boopathiviky
Copy link
Author

#27150

boopathiviky and others added 3 commits October 27, 2023 21:22
adding bucketize to paddle tensor
tensor file has been changed
@nassimberrada
Copy link
Contributor

Hey, thanks for making those changes! The tests seems to fail for all backends with this error hypothesis.errors.InvalidArgument: Cannot sample from a length-zero sequence. indicating there might be an issue with the testing, could you try to take a look and fix that ? Thanks!

@nassimberrada
Copy link
Contributor

Closing Stale PR

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.

5 participants