Conversation
There was a problem hiding this comment.
Pull request overview
Updates DaCe’s SymPy string printer to avoid implicitly printing float literals (e.g., 0.0) as integer literals (0), which can trigger unintended integer promotions/reordering in downstream code generation (notably around max/Max), and adds regression tests.
Changes:
- Modified
DaceSympyPrinter._print_Floatto format floats with trimmed trailing zeros while keeping at least one decimal digit. - Added tests intended to ensure float literals (especially
0.0inmax(a, 0.0)) round-trip throughsymstrwithout demotion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
dace/symbolic.py |
Adjusts Float printing logic in symstr’s printer implementation. |
tests/symbolic_print_test.py |
Adds regression tests around float demotion and Max/max printing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
SymPy printer silently narrows double constants to int, causing non-commutative max behavior DaCe's max wrapper uses variadic templates to bridge Python's loosely-typed frontend to C++. Since std::max requires both arguments to be the same type, the wrapper deduces the type from the first argument and promotes the second argument to match. This is well-defined C++ behavior. This becomes a problem because SymPy's printer implicitly converts double constants to int: a value like 5.0 gets printed as 5 (if you use sympy printer). So instead of generating max(double, double), we emit max(int, double). Combined with the first-argument-biased template deduction, the generated code can silently produce wrong results. To make matters worse, SymPy treats max as commutative and may freely reorder arguments, so the argument order (and therefore the deduced type) is effectively non-deterministic. Possible mitigations: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The symstr might convert
0.0to0. E.g. the iedge below is generated fromlc = max(-zdqs[jl], 0.0), can be converted to use0.I had an interesting butterfly effect. Sympy reordered arguments because the max function is commutative. But not with implicit promotions. If 0 is an integer, and appears first, then the double variable is promoted to int, thus the result will be different from when the double appears first.
I thought it could be related to
sympy_numeric_fix, but I think the fix is in improving the print Float functionsymstr. I propose to change the_print_Floatofsymstrto clean trailing zeroes as before, but ensuring that there is at least one and not converting floats to integers implicitly.With the new print I avoid implicit casts to integer in CloudSC SDFG. (I add some tests too).
See my comment below for a better explanation.