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

Isin extension #5762

Merged
merged 50 commits into from Nov 23, 2022
Merged

Isin extension #5762

merged 50 commits into from Nov 23, 2022

Conversation

pillarxyz
Copy link
Contributor

#5716

added most backend implementations there is only problem with tensorflow I'm still trying to solve since it doesnt have the function isin, once I'm able to do that I will add tests

@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 Ivy API Experimental Run CI for testing API experimental/New feature or request labels Oct 15, 2022
@ivy-leaves ivy-leaves added the Array API Conform to the Array API Standard, created by The Consortium for Python Data API Standards label Oct 16, 2022
@pillarxyz pillarxyz changed the title Isin Isin extension Oct 16, 2022
@abdrahmandiab
Copy link
Contributor

I dont how to deal with these .idea folders getting added to the commit after I solve merge conflicts😅

Hi @pillarxyz, could you tell me what method you use for committing things? is it the terminal or through your IDE source control window? also what IDE do you use? 😅

@pillarxyz
Copy link
Contributor Author

Hi @pillarxyz, could you tell me what method you use for committing things? is it the terminal or through your IDE source control window? also what IDE do you use? 😅

I'm using pycharm with ivy conda environment, and using git in the terminal

@pillarxyz
Copy link
Contributor Author

pillarxyz commented Nov 9, 2022

@abdrahmandiab I think that the tensorflow implementation works fine now, although a particular example keeps failing for some reason even though when I try the function in the python console with the inputs in the examples it delivers the expected results.
--- EDIT
it's the assume_unique argument being weird again I will check on that right now
--- EDIT
I guess the only option left in order to replicate exactly the results from numpy assume_unique argument is to transpile the numpy implementation into tensorflow which will require some other functions that are probably not in tensorflow either.

I think we should probably just merge this if you see fit as everything else except for that argument works perfectly and I will revisit the tensorflow implementation once I figure out a way.

Thanks again for your patience with me and helpful advice

@abdrahmandiab
Copy link
Contributor

abdrahmandiab commented Nov 13, 2022

hi @pillarxyz,
as a last effort, I think the assume_unique output might be producing incorrect results because the generated arrays in these specific test cases themselves are not unique.

So I think before we give up and merge it, if you could edit the data generation to ensure that the generated arrays contain unique elements, I think this might stop the failing examples. My initial idea to do this is to use something like...

dtypes_and_values(...).filter( 
    lambda x: x == np.unique(x) #np.unique returns the unique values in an array
)

And of course let me know if you need any help with this. You can add me on Discord at Diab#2426 as well, I am usually more responsive there! 😅

@abdrahmandiab
Copy link
Contributor

I'm using pycharm with ivy conda environment, and using git in the terminal

Aha, well in this case where you are using it from the terminal, please refrain from using git add . and rather please use git add <specific file name> , this might be a bit tedious but it will save you the trouble of committing unwanted changes that are related to the IDE or to the Array API tests. 😅

@abdrahmandiab abdrahmandiab merged commit 5d49d96 into unifyai:master Nov 23, 2022
@abdrahmandiab
Copy link
Contributor

Hi @pillarxyz,
after so long working on this PR, we've finally done it! 🚀

Thanks so much for your hard work, and good luck with the rest of your application process! <3

@pillarxyz
Copy link
Contributor Author

Hi @pillarxyz, after so long working on this PR, we've finally done it! 🚀

Thanks so much for your hard work, and good luck with the rest of your application process! <3

Thanks alot for the help, I've learnt alot about the project thanks to you <3

@pillarxyz pillarxyz deleted the isin branch November 23, 2022 11:26
KevinUnadkat pushed a commit to KevinUnadkat/ivy that referenced this pull request Dec 4, 2022
Co-authored-by: Abdelrahman Diab <37490334+abdrahmandiab@users.noreply.github.com>
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 API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants