-
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: Added unsorted_segment_mean function #24984
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 Passed!
Hi @rishabgit , |
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.
Hi! I added comments as appropriate and also, the lint test seems to be failing. Could you please use pre-commit for a quick fix?
Feel free to re-request review after making these changes, thanks! 😄
253016f
to
eadac41
Compare
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.
Also, could you please take care of the lint errors? Thanks! 😄 |
15d1f90
to
1a4d817
Compare
9e1dcf6
to
0c6e1e7
Compare
2a48f40
to
45d2734
Compare
I ran the pre-commit commands on my local and all pre-commit tests are passed but GitHub showing an error in lint-code. |
The unsorted_segment_mean function implementation seems correct. But might be a problem with other functions already done in the backend. It also shows an error in my local. Could you please check Rishab what actually the problem is? |
d37878c
to
45d2734
Compare
f040646
to
317f6bb
Compare
Hi @rishabgit, Can u please review the PR and let me know if any changes are needed? Thanks |
This might be because your branch is outdated. Could you please update to latest Ivy and try
Testing results ideally shouldn't change on different machines. Could you please join our discord and ask in appropriate channel if you need further help on this? Also, I see a lot of merge conflicts which I suppose would be resolved when you update to latest Ivy but thought of bringing that up 😅 |
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.
Added review in previous comment :)
ca1aeb1
to
a4e9425
Compare
Hello @rishabgit, I have fixed the lint and merge conflicts. Please let me know what is required to merge this PR. Thank you!! 😄 |
…pr/kamlishgoswami/24984
Close #23779