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

remove reduce ops workaround for keepDim attribute #2001

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

mmanzoorTT
Copy link
Contributor

@mmanzoorTT mmanzoorTT commented Jan 28, 2025

This workaround is still required in case of reducing entire tensor.

closes #1640

@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch 4 times, most recently from c2def74 to 2236c94 Compare January 29, 2025 17:50
@mmanzoorTT mmanzoorTT marked this pull request as ready for review January 29, 2025 17:51
@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch 2 times, most recently from 2e24b3f to ae3c8ae Compare January 30, 2025 14:48
Copy link
Contributor

@sdjordjevicTT sdjordjevicTT left a comment

Choose a reason for hiding this comment

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

Thanks!

@mrakitaTT
Copy link
Contributor

mrakitaTT commented Jan 30, 2025

@mmanzoorTT Thank you for making these changes! Just to make sure, you've verified that tests now pass in debug mode too? :) (because metal fix was unstable in debug mode)

@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch from ae3c8ae to ced1b40 Compare January 30, 2025 23:41
@mmanzoorTT mmanzoorTT requested a review from jnie-TT as a code owner January 30, 2025 23:41
@mmanzoorTT
Copy link
Contributor Author

@mmanzoorTT Thank you for making these changes! Just to make sure, you've verified that tests now pass in debug mode too? :) (because metal fix was unstable in debug mode)

Yes. I tested metal fix in debug mode and all the tests worked :)

@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch from ced1b40 to da50ff3 Compare February 4, 2025 16:27
@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch from da50ff3 to 3c46c36 Compare February 6, 2025 15:22
Copy link
Contributor

@mrakitaTT mrakitaTT left a comment

Choose a reason for hiding this comment

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

Thank you for making these changes Asif!

* Enable workaround for partial and full tensor reduction if 'KeepDim'
  attribute is not supported.
* Enable workaround for full tensor reduction if 'KeepDim' attribute
  is supported.
@mmanzoorTT mmanzoorTT force-pushed the mmanzoor/reduce-op-remove-wa branch from 3c46c36 to 1a92710 Compare February 6, 2025 18:41
@mmanzoorTT mmanzoorTT enabled auto-merge (squash) February 6, 2025 18:43
@mmanzoorTT mmanzoorTT merged commit 4a27a90 into main Feb 6, 2025
30 checks passed
@mmanzoorTT mmanzoorTT deleted the mmanzoor/reduce-op-remove-wa branch February 6, 2025 19:39
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.

Enable Reduce ops silicon tests after Metal reshape issue is fixed
3 participants