You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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
mainalready, and they share theGraphVariables.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.pyandmul.pyobtain both operands viaself._variables.consume_operand_values()and follow the samekeys/values flow as
div.pyandsub.py, instead of pre-committing withget_initializer_value()and treating a computed operand as an error.group + computed column,column + columnandconstant + column.group + single constantbroadcast works; it currently raisesValueErrorin both Add and Mul.
(
add.py:28,mul.py:28).TODO: Implement dividing by a single value, see Div implementationcomments at
add.py:42andmul.py:42are resolved and removed.DivTranslatorandSubTranslatordo.TestAddTranslatorandTestMulTranslatorintests/test_pipeline_steps.pycover every newly supported cell with executed values, mirroring
TestSubTranslator. Note the existingtest_add_second_operand_not_constant/test_mul_second_operand_not_constanttests assert the behaviour that is being removed, so they need to be
replaced.
While touching Mul, two one-line copy-paste defects in
matmul.pyare quickenough 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:44also calls the second operand a "divisor".matmul.py:1has 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
NotImplementedErroreven though the expression istrivially representable in SQL.
Div hit exactly this case with LogisticRegression on double features, which
exports
Div(scores, ReduceSum(Abs(scores))). Supporting it generalized into anormalized 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 canreuse 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. Itreturns column names plus values, with names
Nonefor anything that is nota group of columns.
src/orbital/translation/steps/div.py(DivTranslator.process) andsrc/orbital/translation/steps/sub.py(SubTranslator.process) — the tworeference implementations, both on
main.src/orbital/translation/steps/add.pyandmul.py— same three spots each:"divisor" wording, the TODO, and the missing broadcast.
src/orbital/translation/steps/matmul.py— the two wording defects noted inthe Goal.
tests/test_pipeline_steps.py(TestSubTranslator) — the operand matrixcoverage shape to mirror.
multicolumn support implement ReduceSum and Div multicolumn support #129, support flipped operands in Sub support flipped operands in Sub #127.