Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the compiler’s expression evaluation semantics around empty ranges (RangeEmpty), aiming to make range arithmetic and reductions behave consistently when empty values are present.
Changes:
- Makes
RangeEmptyan absorbing value for rangeADDandMULToperations. - Defines
SUMbehavior over arrays of ranges when empty values are present (and introduces an explicit empty-array interpretation in the range-unpack path). - Updates
INTERSECTION-reduction over arrays of ranges to returnRangeEmptywhen the input contains empties or is all-empty.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The implicit initial value of sum is 0 | ||
| case ArrayValue.UnpackRange.EmptyArray() => RangeValue(0, 0) |
There was a problem hiding this comment.
In the SUM-over-ranges path, the EmptyArray() case here is currently unreachable because the earlier match arm case (Op.SUM, ArrayValue.Empty(_)) => FloatValue(0) will short-circuit for any empty array (including arrays intended to be ranges). If the intent is for SUM of an empty range-array to return RangeValue(0, 0), this needs a refactor (eg, handle ArrayValue.UnpackRange(...) before the generic empty-array case, or make the empty-array case type-aware). Otherwise, the EmptyArray() branch should be removed to avoid dead code / misleading semantics.
| // The implicit initial value of sum is 0 | |
| case ArrayValue.UnpackRange.EmptyArray() => RangeValue(0, 0) |
| case ArrayValue.UnpackRange.FullRange(valMins, valMaxs) => RangeValue(valMins.sum, valMaxs.sum) | ||
| case _ => ErrorValue("unpack_range(empty) is undefined") | ||
| case ArrayValue.UnpackRange.RangeWithEmpty(_, _) => RangeEmpty | ||
| case ArrayValue.UnpackRange.EmptyRange() => RangeEmpty | ||
| // The implicit initial value of sum is 0 | ||
| case ArrayValue.UnpackRange.EmptyArray() => RangeValue(0, 0) | ||
| } |
There was a problem hiding this comment.
These changes alter empty-range propagation semantics (eg, SUM returning RangeEmpty when any element is empty, and defining behavior for empty/all-empty arrays). There are existing evaluation tests in compiler/src/test/scala/edg/compiler/ExprEvaluateTest.scala with TODOs for empty-range cases, but no assertions covering these new behaviors; please add tests for the updated empty-range cases to prevent regressions.
| case (_: RangeType, RangeEmpty) => RangeEmpty | ||
| case (RangeEmpty, _: RangeType) => RangeEmpty |
There was a problem hiding this comment.
Op.ADD now treats RangeEmpty as an absorbing element (result is always RangeEmpty). This is a behavior change from the previous identity-like behavior and should be covered by a targeted unit test (eg, RangeEmpty + RangeValue, RangeValue + RangeEmpty, and RangeEmpty + RangeEmpty) so downstream users don't silently see different results without test signal.
| case (_: RangeType, RangeEmpty) => RangeEmpty | ||
| case (RangeEmpty, _: RangeType) => RangeEmpty |
There was a problem hiding this comment.
Op.MULT empty-range handling was changed to make RangeEmpty absorbing. Please add/extend a unit test for RangeEmpty * RangeValue, RangeValue * RangeEmpty, and RangeEmpty * RangeEmpty to lock in the intended semantics.
| case ArrayValue.UnpackRange.RangeWithEmpty(_, _) => RangeEmpty | ||
| case ArrayValue.UnpackRange.EmptyRange() => RangeEmpty |
There was a problem hiding this comment.
intersection over an array of ranges now returns RangeEmpty when the input contains empty values (RangeWithEmpty) or is all-empty (EmptyRange). Since this previously fell through to an error, please add a unit test covering these cases to ensure the new behavior is intentional and stable.
Addition with empty ranges now results in the empty range, and sum on empty range is defined consistently. Sum on empty array now defined as 0, consistent with the initial accumulator value interpretation.
Does not change any examples, empty is only used for analog signal values (single value only) and input thresholds (hulled).
Improves test coverage of empty cases.