-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 extra arguments to min #27152
Conversation
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Aaryan562, just requested a minor change. As far as the test failures go, most of them seem to be stemming from the new paddle
release which is unrelated to your PR. The tests for test_min
seem to be passing. Thanks 😄
hint since default value was not None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Aaryan562, just requested another change, thanks 😄
ivy_tests/test_ivy/test_functional/test_core/test_statistical.py
Outdated
Show resolved
Hide resolved
Happy to take a look again once you're done with the changes 😄 |
ivy_tests/test_ivy/test_functional/test_core/test_statistical.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just requested a final couple of changes, rest looks good to merge. Thanks @Aaryan562 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just requested a change in one of the comments from the previous review, rest looks good. thanks @Aaryan562 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, but the test seems to be failing with the jax
and numpybackends. For the
jaxofc, the test is failing due to the version number, but for the
numpy` backend, the failure seems to be related to the changes made. Could you please look into it? Thanks @Aaryan562 😄
@vedpatwardhan I did the changes as you said in the last commit. But it did gave the error and I have mentioned it in the above msg as well which I think got missed. |
Sorry about that, just responded to that message, feel free to request a review once you're done with those changes. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Feel free to merge, thanks @Aaryan562 😄
@vedpatwardhan As discussed in the trello card where I need to add additional arguments to the statistical functions, I decided to create individual PR for each function as it was becoming overwhelming for me to do all the changes in single PR and also it would be easy for you to debug and review.