Skip to content

Align Add and Mul translators with the consolidated Div/Sub implementation #126

Description

@amol-

Goal

Add and Mul translate their operands through the same normalized code path
Div and Sub already use, so each operand can independently be a group of
columns, a single column, a constant scalar or a constant list.

Div (#129) and Sub (#127) are both on main already, and they share the
GraphVariables.consume_operand_values() helper that does the normalization.
Add and Mul are the two remaining translators still carrying the pre-refactor
shape.

Done when:

  • add.py and mul.py obtain both operands via
    self._variables.consume_operand_values() and follow the same
    keys/values flow as div.py and sub.py, instead of pre-committing with
    get_initializer_value() and treating a computed operand as an error.
  • Each supports the operand matrix Div and Sub support, including
    group + computed column, column + column and constant + column.
  • group + single constant broadcast works; it currently raises ValueError
    in both Add and Mul.
  • The error text no longer calls the second operand a "divisor"
    (add.py:28, mul.py:28).
  • The TODO: Implement dividing by a single value, see Div implementation
    comments at add.py:42 and mul.py:42 are resolved and removed.
  • Class docstrings state the supported operand matrix, as DivTranslator and
    SubTranslator do.
  • TestAddTranslator and TestMulTranslator in tests/test_pipeline_steps.py
    cover every newly supported cell with executed values, mirroring
    TestSubTranslator. Note the existing
    test_add_second_operand_not_constant / test_mul_second_operand_not_constant
    tests assert the behaviour that is being removed, so they need to be
    replaced.

While touching Mul, two one-line copy-paste defects in matmul.py are quick
enough to fix in the same pass. They are wording only; MatMul itself keeps
requiring initializer values, because it needs the actual weight matrix rather
than an expression.

  • matmul.py:44 also calls the second operand a "divisor".
  • matmul.py:1 has the wrong module docstring: it says
    """Implementation of the LabelEncoder operator.""".

Why

skl2onnx and the PyTorch exporter emit Add/Sub/Mul nodes whose second operand
is a value computed by a previous node rather than a constant. Add and Mul
currently fail with NotImplementedError even though the expression is
trivially representable in SQL.

Div hit exactly this case with LogisticRegression on double features, which
exports Div(scores, ReduceSum(Abs(scores))). Supporting it generalized into a
normalized operand path that removed the branching entirely: Div went from 108
to 89 lines while gaining six operand combinations. Sub then reused the same
path, and the normalization was extracted into
GraphVariables.consume_operand_values() so both share it. Add and Mul can
reuse the same helper, and their TODO comments already point at Div as the
reference implementation.

References

  • src/orbital/translation/variables.py
    (GraphVariables.consume_operand_values) — the shared helper to reuse. It
    returns column names plus values, with names None for anything that is not
    a group of columns.
  • src/orbital/translation/steps/div.py (DivTranslator.process) and
    src/orbital/translation/steps/sub.py (SubTranslator.process) — the two
    reference implementations, both on main.
  • src/orbital/translation/steps/add.py and mul.py — same three spots each:
    "divisor" wording, the TODO, and the missing broadcast.
  • src/orbital/translation/steps/matmul.py — the two wording defects noted in
    the Goal.
  • tests/test_pipeline_steps.py (TestSubTranslator) — the operand matrix
    coverage shape to mirror.
  • Implement ReduceSum translation step Implement ReduceSum translation step #109, implement ReduceSum and Div
    multicolumn support implement ReduceSum and Div multicolumn support #129, support flipped operands in Sub support flipped operands in Sub #127.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions