-
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
tolist #21303
tolist #21303
Conversation
Thanks for contributing to Ivy! 😊👏 |
If you are working on an open task, please edit the PR description to link to the issue you've created. For more information, please check ToDo List Issues Guide. Thank you 🤗 |
I did , is this proper? |
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.
Please remove editor specific files (.idea/*
) from the PR
I pushed after making the changes , do I need to create another PR? |
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.
Did that
You don't need to create another PR. |
I removed the .idea directory and also from .gitignore , is it proper now? |
@vaithak sir? |
|
||
@to_ivy_arrays_and_back | ||
def to_list(x): | ||
return x.tolist() |
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.
why do we need this extra function?
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.
It was pasted again at the end by mistake , I corrected it.
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.
I pushed again. @vaithak
You need to remove only your changes from the |
@Sanchay-T please rebase your changes on master and make sure to not make any changes in the |
) | ||
@to_ivy_arrays_and_back | ||
def tolist(x): | ||
return ivy.tolist(x) |
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, ivy.tolist
has no implementation in the functional layer so this won't work. Have you tested this locally?
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.
No , I had trouble checking it locally , as when I imported the file on top and ran , it kept on looks at the ivy package that was installed through req I guess
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.
Should I just delete this all and start again?
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.
How will starting over again help? You need to implement the corresponding function in ivy's functional layer and maybe in the backend too. Please read through ivy's documentation to get an understanding on this.
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.
I will go through the documentation once again , but I understood the paddle implementation. I cannot seem to find the implementation of tolist in ivy. Maybe I am wrong and sorry for troubling you. But I know that my function will take the tensor as an input and the output should be the list. But is there something inbuilt?
This is the paddle paddle documentation:
And this is what I found in the source code on github of Paddle paddle :
I still dont understand the tolist() declaration. It would help me a lot if you can somewhat guide me?
Thankyou
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.
I will say the same thing again, go through ivy's documentation to understand what you are missing.
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.
Yes went thorough the entire documentation , understood what I was doing wrong. Thankyou. After I push the corrected code here , is the octernship task complete. I didnt understand where to exactly create a pr , in the webinar on youtube they said , there is another repo that will be created by the ivy team for me , and there I have to push
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 is the issue still allocated to me how do I check , because in the documentation it was mentioned 7 days
@vaithak sir , I made the proper correction and imported the function and checked it , it is working , but I am getting assertion error while running the tests and I am not able to figure that out. Can you help me , please. I am really trying |
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.
I have fixed the .idea
folder and .gitignore
which I had requested for multiple times.
The test is failing for all the backends: AssertionError: The length of results from backend numpy and ground truthframework paddle does not match
.
Please make sure these are fixed locally.
Thankyou @vaithak , I modified the code checked in my local system. I am not understanding this error. Rest 3 test cases passed successfully : [FAILED ivy_tests/test_ivy/test_frontends/test_paddle/test_tensor/test_manipulation.py::test_paddle_tolist[cpu-numpy-False-False] - AssertionError: The length of results from backend numpy and ground truthframework paddle does not match |
Implemented tolist function for PaddlePaddle backend. Fixes #21303. |
Implemented tolist function for PaddlePaddle backend. Fixes #19170 |
close #21956