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.

Sign up for GitHub

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

Jump to bottom

atleast 3d extension #6976

Merged
merged 8 commits into from
Nov 25, 2022
Merged

atleast 3d extension #6976

merged 8 commits into from
Nov 25, 2022

Conversation

raghuveerbhat
Copy link
Contributor

Close #6886
And also fix for test failing for #6883

@ivy-leaves ivy-leaves added the Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards label Nov 13, 2022
@raghuveerbhat raghuveerbhat marked this pull request as ready for review November 13, 2022 18:01
@raghuveerbhat raghuveerbhat changed the title atleast 3d extension and fix for test atleast 3d extension Nov 13, 2022
@atakanoc
Copy link
Contributor

Hey @raghuveerbhat!

This pull request looks great! Just a couple of notes, though. ☺️

  1. The typehints for the parameters don't seem to be quite right. Alongside Arrays, a user is able to pass ints, floats and so on directly.
  2. There seem to be some changes made to the test_atleast_2d even though that function is out of scope for this task. Please revert that.

Once you're done making these changes, feel free to run merge_with_upstream.sh on your branch and then push the changes. Thanks! ☺️

@ivy-leaves ivy-leaves added Ivy API Experimental Run CI for testing API experimental/New feature or request and removed Experimental labels Nov 18, 2022
@raghuveerbhat
Copy link
Contributor Author

raghuveerbhat commented Nov 19, 2022 •
edited
Loading

Hi @atakanoc , sorry for the delay in reply.. got caught up in some work. Thank you for your review. I agree with all your comments and I have made required changes. Hope I can ignore lint error as it is not related to my PR.

Let me know if any other changes are required :)

@atakanoc
Copy link
Contributor

No worries at all! Thank you again, I'm giving this a check now.

@atakanoc atakanoc merged commit 6c1a318 into ivy-llc:master Nov 25, 2022
@atakanoc
Copy link
Contributor

Looks great, thanks a lot! I think you missed the PyTorch typehints but I've handled that part myself, so no worries.

Have a good one! ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@atakanoc atakanoc

Labels
Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Ivy API Experimental Run CI for testing API experimental/New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

atleast_3d
3 participants
@raghuveerbhat @atakanoc @ivy-leaves