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

Implemented sinc in the backend #4436

Closed
wants to merge 8 commits into from
Closed

Implemented sinc in the backend #4436

wants to merge 8 commits into from

Conversation

pj-mathematician
Copy link
Contributor

Can you review and please list the additions/deletions I should incorporate?
And I will also need help with the doc string for ivy/elementwise.py

Thanks

@ivy-leaves ivy-leaves added Ivy Functional API Function Reformatting Reformat all Ivy functions in accordance with the latest coding style in the contributor guide labels Sep 14, 2022
@Unevilicorn
Copy link
Contributor

Hi,

Could you also link the issue this relates to. This is so that it's easier for us to track what's being done.
The test is failing for sinc, you can see by going through the checks near the bottom of this page, click on detail on the row for test-core-ivy / run-nightly-tests ({backend}, elementwise) then click on Run Functional-Core Test

Thanks

@pj-mathematician
Copy link
Contributor Author

@Unevilicorn thank you for the review, I will fix the tests by EOD.

sinc is a part of the functional API and is referenced in the Frontend TODO lists #12 , #1525 and #3612
But when I started the Frontend sinc I noticed that it isn't present in the backend at all. Hence I wrote the backend implementations. I believe it should be added to the list here #3856 . I have opened a separate issue to track this here #4447

@ivy-leaves ivy-leaves added the Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards label Sep 14, 2022
@Unevilicorn
Copy link
Contributor

I would actually recommend doing this compositionally then, since this function isn't too complex to compose, you can see it's behaviour from pytorch docs.

I'll take another look, and check if this should be added to the list 👍

@pj-mathematician
Copy link
Contributor Author

I understand. So instead of adding any backend functions, I should implement sinc compositionally to only the frontend files of Numpy and Torch, as mentioned in the frontend todo tasks, right?

@Unevilicorn
Copy link
Contributor

Yeah, that's the simplest way to go about it, just implementing them compositionally in the frontend for your framework. However it does feel like it could be added as an extension, I'll let you know when I find out if this should be added or not.

@pj-mathematician
Copy link
Contributor Author

pj-mathematician commented Sep 14, 2022

Alright then! Closing this PR for now, and will work on the frontend as you suggested. Let me know if there is a need for it in the backend, I'd love to get back on it 🙌🏻

Thanks alot for your help!!

@Unevilicorn
Copy link
Contributor

Hi @pj-mathematician,
I've been told that this function should be added to Ivy and it's now on the extension list. If you would like, you can continue this PR and try to implement it, otherwise you can just leave this closed and someone else can pick up that task.
Thanks

@pj-mathematician
Copy link
Contributor Author

@Unevilicorn , I have opened #4462 to work on this. Thank you for the clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards Function Reformatting Reformat all Ivy functions in accordance with the latest coding style in the contributor guide Ivy Functional API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants