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

torch frontend asinh and asinh_ instance method #5725

Merged
merged 6 commits into from
Oct 17, 2022
Merged

torch frontend asinh and asinh_ instance method #5725

merged 6 commits into from
Oct 17, 2022

Conversation

raghuveerbhat
Copy link
Contributor

No description provided.

@raghuveerbhat
Copy link
Contributor Author

raghuveerbhat commented Oct 14, 2022 •
edited
Loading

close #5723
close #5771

@ivy-leaves ivy-leaves added the PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist label Oct 14, 2022
@raghuveerbhat raghuveerbhat changed the title torch frontend asinh instance method torch frontend asinh and asinh_ instance method Oct 15, 2022
@raghuveerbhat
Copy link
Contributor Author

Hi @Nightcrab , there are couple of lint issues which are unrelated to my changes. Hope I can ignore:)

With respect to failing test case for asinh_ (and couple of other functions unrelated to my changes), I've found some change done in this commit 96f0cac which might be causing the issue. The problematic code is this line. I think this function name aliasing might not be needed. I tested by removing that line in my local system, and the test cases for test_creation_ops and test_tensor pass.

Since I'm new contributor to the ivy project, I wanted to get your opinion on this. Also, if it is indeed a bug, should I raise new issue and fix it in this pull request or can I fix it without raising the new issue. Sorry for bothering you with many questions.

@Nightcrab Nightcrab self-requested a review October 17, 2022 00:57
@Nightcrab
Copy link
Contributor

Removing that line is fine, since the tensor function is provided in Tensor.py in a more correct manner. To keep repo structure consistent, I would move that alias out of Tensor.py and into creation_ops.py but that's outside of the scope of this PR.

@Nightcrab Nightcrab merged commit b53d334 into ivy-llc:master Oct 17, 2022
Nightcrab
Nightcrab reviewed Oct 17, 2022
View reviewed changes
Copy link
Contributor

@Nightcrab Nightcrab left a comment

Choose a reason for hiding this comment

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

merged since everything looks fine + tests pass

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

@Nightcrab Nightcrab Nightcrab left review comments

Assignees

@Nightcrab Nightcrab

Labels
PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raghuveerbhat @Nightcrab @ivy-leaves