Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kaushikcfd opened this issue Oct 29, 2021 · 7 comments
Open

Comments

@kaushikcfd
Copy link
Collaborator

>>> 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
Copy link
Owner

inducer commented Oct 29, 2021

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

inducer commented Oct 29, 2021

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
Copy link
Collaborator Author

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
Copy link
Owner

inducer commented Oct 29, 2021

OK, I can live with that.

@kaushikcfd
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants