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

Revert 'reduce_avg' attribute from 'dim_arg' to 'dim' and update op_mapping #1129

Merged
merged 3 commits into from
Jan 30, 2025

Conversation

mstojkovicTT
Copy link
Contributor

@mstojkovicTT mstojkovicTT commented Jan 29, 2025

Problem description

The previous change in PR #1101 modified the named attribute of reduce_avg from dim to dim_arg to align with tt-mlir naming conventions. However, this change has been identified as problematic and needs to be reverted.

What's changed

This PR reverts the changes introduced in PR #1101, restoring the named attribute of reduce_avg back to dim. Additionally, the op_mapping has been updated from dim to dim_arg to ensure compatibility with ttir.

Checklist

  • New/Existing tests provide coverage for changes

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests501 ran436 passed65 skipped0 failed
TestResult
No test annotations available

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests560 ran484 passed76 skipped0 failed
TestResult
No test annotations available

Copy link
Contributor

@nvukobratTT nvukobratTT left a comment

Choose a reason for hiding this comment

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

For clarify, can we update attribute names in the op definition as well? E.g.

forge/forge/op/reduce.py

@mstojkovicTT mstojkovicTT force-pushed the mstojkovic/reduce_avg_attr_update branch from c1fe80f to d6cbcf9 Compare January 30, 2025 08:47
@mstojkovicTT
Copy link
Contributor Author

For clarify, can we update attribute names in the op definition as well? E.g.

Correct me if I am wrong, but ReduceAvg seems to have correct namings (dim was not changed from dim_arg to dim here)

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests555 ran474 passed81 skipped0 failed
TestResult
No test annotations available

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests496 ran436 passed60 skipped0 failed
TestResult
No test annotations available

@nvukobratTT
Copy link
Contributor

nvukobratTT commented Jan 30, 2025

For clarify, can we update attribute names in the op definition as well? E.g.

Correct me if I am wrong, but ReduceAvg seems to have correct namings (dim was not changed from dim_arg to dim here)

Did you spot this line: dim_arg=[dim] ?

def ReduceAvg(name: str, operandA: Tensor, dim: int, keep_dim: bool = True) -> Tensor:
    """
    Reduce by averaging along the given dimension

    Parameters
    ----------
    name: str
        Op name, unique to the module, or leave blank to autoset

    operandA: Tensor
        First operand

    dim: int
        Dimension along which to reduce. A positive number 0 - 3 or negative from -1 to -4.

    Returns
    -------
    Tensor
        Forge tensor
    """

    assert (dim >= -4) and (dim <= 3)
    # if dim < 0:
    #     dim += 4

    return op("reduce_avg", name, operandA, attrs=(dim, keep_dim), dim_arg=[dim], keep_dim=keep_dim).get_tensor()

@mstojkovicTT mstojkovicTT force-pushed the mstojkovic/reduce_avg_attr_update branch from d6cbcf9 to 5061845 Compare January 30, 2025 10:14
@mstojkovicTT
Copy link
Contributor Author

mstojkovicTT commented Jan 30, 2025

For clarify, can we update attribute names in the op definition as well? E.g.

Correct me if I am wrong, but ReduceAvg seems to have correct namings (dim was not changed from dim_arg to dim here)

Did you spot this line: dim_arg=[dim] ?

def ReduceAvg(name: str, operandA: Tensor, dim: int, keep_dim: bool = True) -> Tensor:
    """
    Reduce by averaging along the given dimension

    Parameters
    ----------
    name: str
        Op name, unique to the module, or leave blank to autoset

    operandA: Tensor
        First operand

    dim: int
        Dimension along which to reduce. A positive number 0 - 3 or negative from -1 to -4.

    Returns
    -------
    Tensor
        Forge tensor
    """

    assert (dim >= -4) and (dim <= 3)
    # if dim < 0:
    #     dim += 4

    return op("reduce_avg", name, operandA, attrs=(dim, keep_dim), dim_arg=[dim], keep_dim=keep_dim).get_tensor()

Right, i missed that, thanks!

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests496 ran436 passed60 skipped0 failed
TestResult
No test annotations available

Copy link

TestsPassed ✅Skipped ⚠️Failed
TT-Forge-FE Tests555 ran474 passed81 skipped0 failed
TestResult
No test annotations available

@mstojkovicTT mstojkovicTT merged commit 05094f3 into main Jan 30, 2025
6 checks passed
@mstojkovicTT mstojkovicTT deleted the mstojkovic/reduce_avg_attr_update branch January 30, 2025 12:58
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.

3 participants