inducer / pymbolic

A simple package to do symbolic math (focus on code gen and DSLs)
http://mathema.tician.de/software/pymbolic
Other
106 stars 25 forks source link

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

Open kaushikcfd opened 2 years ago

kaushikcfd commented 2 years ago
>>> 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)'
inducer commented 2 years ago

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?

kaushikcfd commented 2 years ago

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 commented 2 years ago

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 ;).

inducer commented 2 years ago

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?

kaushikcfd commented 2 years ago

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.

inducer commented 2 years ago

OK, I can live with that.

kaushikcfd commented 1 year ago

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