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

Implement conversions for stablehlo logistic, tan, and tanh lops #1122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

uazizTT
Copy link
Contributor

@uazizTT uazizTT commented Oct 31, 2024

No description provided.

Copy link
Contributor

@mmanzoorTT mmanzoorTT left a comment

Choose a reason for hiding this comment

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

I'll suggest to add these ops in alphabetical order now and avoid refactoring later.

@uazizTT
Copy link
Contributor Author

uazizTT commented Oct 31, 2024

I'll suggest to add these ops in alphabetical order now and avoid refactoring later.

Yes I tried with my last PR here but I found that due to large volume of PRs going in adding new ops, it causes a lot of merge conflicts and also makes it difficult for reviewers to read due to code movement. Secondly, I see others merging PRs without sorting them in alphabetical order. So either the reviewers enforce it for each PR going forward to avoid repeated merged conflicts or else we can refactor once at the end.

Copy link
Contributor

@tapspatel tapspatel left a comment

Choose a reason for hiding this comment

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

looks great! Thank you for updating the silicon tests with your changes

@jserbedzijaTT
Copy link
Contributor

jserbedzijaTT commented Nov 1, 2024

I am already implementing the log operation as described in this issue. In the future, please check if there is already an issue for the operation you are implementing, as this is becoming too chaotic.

@uazizTT
Copy link
Contributor Author

uazizTT commented Nov 1, 2024

I am already implementing the log operation as described in this issue. In the future, please check if there is already an issue for the operation you are implementing, as this is becoming too chaotic.

Hey @jserbedzijaTT I think it the other way around, @mrakitaTT created the issue last week which is assigned to me. I think it is important to make sure that an issue is not created already before opening a new one @pilkicTT.

@sdjordjevicTT
Copy link
Contributor

I am already implementing the log operation as described in this issue. In the future, please check if there is already an issue for the operation you are implementing, as this is becoming too chaotic.

Hey @jserbedzijaTT I think it the other way around, @mrakitaTT created the issue last week which is assigned to me. I think it is important to make sure that an issue is not created already before opening a new one @pilkicTT.

Hi @uazizTT, this is the core of the issue: we have multiple places where we create issues for new ops, which is causing this kind of confusion. I shared detailed instructions on our Slack channel on how to handle these types of situations moving forward. If anything is unclear, please feel free to reach out to me.

@uazizTT uazizTT force-pushed the uaziz/stablehlo-logistic-op branch 2 times, most recently from 4ca901d to b61a21e Compare November 7, 2024 20:14
@uazizTT uazizTT changed the title Implement conversions for stablehlo logistic, tan, tanh and log ops Implement conversions for stablehlo logistic, tan, and tanh lops Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants