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

Symbol specialization in auto_optimizer() never took effect. #1410

Merged
merged 4 commits into from
Nov 14, 2023

Conversation

philip-paul-mueller
Copy link
Collaborator

The dict that was storing all the symbols, known_symbols, was emptied just after its creation. The efect was that the specialization never took effect.

philip-paul-mueller and others added 3 commits October 26, 2023 08:12
The `dict` that was storing all the symbols, `known_symbols`, was emptied just after its creation.
The efect was that the specialization never took effect.
After looking again at the code I realized that I should have deleted the other definition of `known_symbols`.
@phschaad
Copy link
Collaborator

I'm a bit confused, the change (now) seems to be the exact inverse of what it originally was. While I see that this line should be removed, shouldn't this have no effect on behavior whatsoever since it is effectively a dead (or succeeded) line of code?

@philip-paul-mueller
Copy link
Collaborator Author

I think the difference is because of the following loop (see file dace/transformation/auto/auto_optimize.py, line 650ff).
There on line 653 it is checked if the value v is a sympy.Integer and if so it tries to transform it into a Python int, in case this fails the symbol is ignored (I am not sure why though), and will thus not enter known_symbols.
Therefore known_symbols will only contain Python float and int.

However, if known_symbols is first directly constructed, this transformation is not done, and the "offending" (in the sense of having type sympy.Integer which can not be transformed to int) symbols will remain inside the dict.
While the loop will replace all sympy.Integers that can be transformed to Python int with a corresponding Python int it will not remove the sympy.Integer for which this transformation fails and this probably causes the issues we are seeing.

@BenWeber42
Copy link
Contributor

I agree. Basically, if you only look at these two lines, it would seem that the reset should be removed (as initially proposed):

known_symbols = {s: v for (s, v) in symbols.items() if s in sdfg.free_symbols}
known_symbols = {}

However, looking at the larger context, it becomes clear that the dict comprehension of the first initialization is a strict super set of the later filtering code:

known_symbols = {s: v for (s, v) in symbols.items() if s in sdfg.free_symbols}
known_symbols = {}
for (s, v) in symbols.items():
if s in sdfg.free_symbols:
if isinstance(v, (int, float)):
known_symbols[s] = v
if isinstance(v, sympy.Integer):
try:
known_symbols[s] = int(v)
except TypeError:
pass

And this is in line with the doc-string:

:param symbols: Optional dict that maps symbols (str/symbolic) to int/float

So it seems like it's best to initialize empty and remove the redundant dict comprehension.

@BenWeber42 BenWeber42 requested a review from phschaad November 13, 2023 15:51
@phschaad phschaad enabled auto-merge (squash) November 13, 2023 21:34
@phschaad phschaad merged commit e5b64bf into spcl:master Nov 14, 2023
9 checks passed
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

Successfully merging this pull request may close these issues.

3 participants