pymbolic icon indicating copy to clipboard operation
pymbolic copied to clipboard

StringifyMapper: Incorrect result for side-by-side product nodes

Open kaushikcfd opened this issue 3 years ago • 7 comments

>>> import pymbolic.primitives as p
>>> a = p.Variable("a")
>>> b = p.Variable("b")
>>> c = p.Variable("c")
>>> print(a * (b * c))
a*b*c  # Should have been 'a*(b*c)'

kaushikcfd avatar Oct 29 '21 00:10 kaushikcfd

Pymbolic flattens those, assuming that multiplication is associative. (Product can have multiple children.) Do you have a use case in which that's not correct behavior?

inducer avatar Oct 29 '21 00:10 inducer

Pymbolic flattens those,

The AST is correct (i.e. unflattened) but I think the StringifyMapper is to be blamed here.:

>>> a * (b * c)
Product((Variable('a'), Product((Variable('b'), Variable('c')))))

Do you have a use case in which that's not correct behavior?

No, no concrete use case.

kaushikcfd avatar Oct 29 '21 00:10 kaushikcfd

No, no concrete use case.

Well given floating point arithmetic is not associative we can always buggy behavior associated with this in a generated code ;).

kaushikcfd avatar Oct 29 '21 00:10 kaushikcfd

Well given floating point arithmetic is not associative we can always buggy behavior associated with this in a generated code ;).

Grr, you're right. We should probably turn off auto-flattening. #69

For the StringifyMapper, I can think of use cases where I'd want the parentheses and where I wouldn't. (Math vs CS, really.) Would an option be OK?

inducer avatar Oct 29 '21 01:10 inducer

I can think of use cases where I'd want the parentheses and where I wouldn't. (Math vs CS, really.) Would an option be OK?

I think its unsafe to stringify to incorrect schedules and should be avoided.

Would an option be OK?

Yep, but the default should prefer correctness.

kaushikcfd avatar Oct 29 '21 02:10 kaushikcfd

OK, I can live with that.

inducer avatar Oct 29 '21 03:10 inducer

I think it is not just StringifyMapper that is wrong, even the parser implements left-to-right associativity incorrectly:

>>> parse("a*b*c")
Product((Variable('a'), Product((Variable('b'), Variable('c')))))
>>> from pymbolic.mapper.evaluator import evaluate
>>> a_np = np.float64(1/3)
>>> b_np = np.finfo("float64").max
>>> c_np = np.float64(2)
>>> a_np * b_np * c_np
1.1984620899082103e+308
>>> evaluate(parse("a*b*c"), {"a": a_np, "b": b_np, "c": c_np})
/home/line/.local/lib/python3.10/site-packages/pytools/__init__.py:1103: RuntimeWarning: overflow encountered in double_scalars
  return reduce(mul, iterable, 1)
inf


kaushikcfd avatar Nov 20 '22 01:11 kaushikcfd