-
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
feat: Added ref method to TensorFlow Frontend #26316
Conversation
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.
PR Compliance Checks Passed!
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.
Congrats on making your first Pull Request and thanks for supporting Ivy! 🎉
Join the conversation in our Discord
Here are some notes to understand our tests:
- We have merged all the tests in one file called `display_test_results` job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that fails on this PR.
Please make sure they are passing. 💪
Keep in mind that we will assign an engineer for this task and they will look at it based on the workload that they have, kindly be patient 😄.
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.
Hi, seems like you have changed docs/demos
which you might have done by mistake. Can you revert that change because we can't merge with that change in place?
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 added the ref() function method for the code number lines 59, 60 and 61. It requires ref() method to be added into the code for Tensorflow Frontend application for the issue #26316
@@ -53,6 +53,9 @@ def shape(self): | |||
def get_shape(self): | |||
return tf_frontend.raw_ops.Shape(input=self) | |||
|
|||
def ref(self): | |||
return self._ivy_array.ref() | |||
|
|||
def set_shape(self, shape): | |||
if shape is None: | |||
return |
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.
def ref(self, shape):
if shape is None:
return self._ivy_array.ref()
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.
Hey @Kamalesh3112
Thanks for reviewing! :)
But, according to the TensorFlow API docs, ref
method doesn't have any parameters. 😅
Did you add that by mistake or does it point to something that needs to be improved?
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.
Yeah, the ref() method doesn't have any parameters and it has to be improved by adding those particular lines of code.
This reverts commit 32dfce66a478360e4a9897e71028b147da752b2b.
c07240a
to
8d5cc46
Compare
This reverts commit 8d5cc46.
@KareemMAX Sorry for those extra revert commits as I'm learning to figure it out. 😅 Also, do the failed checks from above need to be resolved? |
Hi @ShruAgarwal , |
1 similar comment
Hi @ShruAgarwal , |
Hey @NiteshK84, |
Hello, I think this PR is unactive for a little while, so I'll close it for now. If you want to continue working on it in the future please feel free to open it and start working. Thanks and happy contributing 😊 |
PR Description
Implemented an Instance Method named,
ref
to TensorFlow FrontendI tested running the function over all backends but, unfortunately, it fails for all of them except for
tensorflow
. Later, I came to know thatref
method or any such similar method doesn't exist in any other backend frameworks. 😅Related Issue
Pending issue to be resolved #26516
Close #26140
Checklist
Please review the changes and let me know your thoughts if I'm correct or not. 🙏