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

added ifftn to ivy functional api #17397

Merged
merged 7 commits into from
Jun 22, 2023
Merged

added ifftn to ivy functional api #17397

merged 7 commits into from
Jun 22, 2023

Conversation

akshatvishu
Copy link
Contributor

Close #17393

@ivy-leaves ivy-leaves added 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 PaddlePaddle Backend labels Jun 20, 2023
KareemMAX
KareemMAX suggested changes Jun 20, 2023
View reviewed changes
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.

Great job @akshatvishu 🚀

I've left some formatting comments, otherwise everything should be good to go after we run the test

ivy_tests/test_ivy/test_functional/test_experimental/test_nn/test_layers.py Outdated Show resolved Hide resolved
ivy/data_classes/container/experimental/layers.py Outdated Show resolved Hide resolved
ivy/data_classes/container/experimental/layers.py Outdated Show resolved Hide resolved
ivy/functional/ivy/experimental/layers.py Outdated Show resolved Hide resolved
@akshatvishu
Copy link
Contributor Author

Please let it

Dog LGTM Keyboard

KareemMAX
KareemMAX suggested changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@KareemMAX KareemMAX left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please let it

Dog LGTM Keyboard

Wish I could 😅. As we are trying to adhere to PEP 257 I need the issues you resolved to be fixed before merging. In the meantime I'm reviewing the ifftn input space to make sure that your test is exhaustive, if it is and it passes the PR will be good to go.

@KareemMAX
Copy link
Contributor

Just a quick side note, all tests have passed. Once you make the docstring changes your PR is good to merge

@akshatvishu
Copy link
Contributor Author

Wish I could sweat_smile. As we are trying to adhere to PEP 257 I need the issues you resolved to be fixed before merging. In the meantime I'm reviewing the ifftn input space to make sure that your test is exhaustive, if it is and it passes the PR will be good to go.

Ahh, So sorry @KareemMAX I found where the mistake is happening. When I try to change the docstring as you suggested the pre-commit hook for docformatter fails.

image

And then this hook reformat my doc to its current state back again from the changes that you suggested and thus, changes you suggested aren't reflected when I push the code.
I was so confused because I did make the changes that you suggested! Should I skip the pre-commit hook entirely to apply changes that you suggested?

@akshatvishu
Copy link
Contributor Author

I made the changes to doc as you suggested by skipping pre-commit

@KareemMAX
Copy link
Contributor

@akshatvishu I'm guessing you have missed the blank line between summary and description. Which confused pre-commit

KareemMAX
KareemMAX suggested changes Jun 22, 2023
View reviewed changes
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.

Seems like it was an issue from my side 😅😅

Just apply those suggestions and commit them view pre-commit and you are good to go

Sorry 😅😅

ivy/data_classes/container/experimental/layers.py Show resolved Hide resolved
ivy/data_classes/container/experimental/layers.py Show resolved Hide resolved
akshatvishu and others added 2 commits June 22, 2023 22:02
@akshatvishu @KareemMAX
Update ivy/data_classes/container/experimental/layers.py
c117c8d 
Co-authored-by: Kareem Morsy <kreem.morsy@hotmail.com>
@akshatvishu @KareemMAX
Update ivy/data_classes/container/experimental/layers.py
f513a07 
Co-authored-by: Kareem Morsy <kreem.morsy@hotmail.com>
KareemMAX
KareemMAX approved these changes Jun 22, 2023
View reviewed changes
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.

LGTM! finally 😂

Merging now

@KareemMAX KareemMAX merged commit 05cd6af into ivy-llc:master Jun 22, 2023
@akshatvishu
Copy link
Contributor Author

akshatvishu commented Jun 23, 2023 •
edited
Loading

LGTM! finally joy

Merging now
Thanks for the review Kareem!

Kid Pumped

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

@KareemMAX KareemMAX KareemMAX approved these changes

Assignees

@KareemMAX KareemMAX

@MahmoudAshraf97 MahmoudAshraf97

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 Ivy Functional API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ifftn
4 participants
@akshatvishu @KareemMAX @MahmoudAshraf97 @ivy-leaves