feat[dace][next]: Deterministic gt_split_access_nodes()
#2383
Conversation
|
Performance seems to be okay: 
|
edopao
left a comment
edopao
left a comment
There was a problem hiding this comment.
LGTM, only some minor comments/questions.
| ) | ||
|
|
||
| # Since the transformation only applies to single use data, the order in which the | ||
| # states are processed is irrelevant. Furthermore, the fragments generated through |
There was a problem hiding this comment.
"Furthermore, the fragments generated..." I don't see how this is relevant. Are you considering the case in which gt_split_access_nodes() is called multiple times?
There was a problem hiding this comment.
This is the justification why scanning the state once is enough.
Because you could argue that you should examine the result of the split (which generates more AccessNodes) should be examined as well.
And we would thus need an interative procedure.
However, I am not fully sure now if this might be needed.
I will update the comment to make this more clear.
| # NOTE: Turning them into a string is the best solution is probably the only way | ||
| # to achieve some stability. The only downside is that the order now depends | ||
| # on the specialization level that is used, i.e. to we have numbers or symbols. | ||
| split_description = sorted(split_description, key=lambda split: str(split)) |
There was a problem hiding this comment.
split_description is a Sequence, so it is already sorted. As alternative, we could ensure the deterministic order in the place where the sequence is created.
There was a problem hiding this comment.
I do not fully understand your comment, are you saying that sorting is not needed, because it is a sequence?
According to the Python glossary a Sequence extends an Iterable by an efficient __getitem__() function, which essentially give sense to the concept of "first", "second", etc. element.
This means that they have some (fixed but ultimately random) order but to be stable you need a specific order.
There was a problem hiding this comment.
On the second thought, I agree with sorting this sequence here. My motivation is that this function should in theory implement some logic for the order, but we rely on alphabetical order for now.
There was a problem hiding this comment.
I agree this is what the Note should express, that this is the only way I found that gives you something stable.
Because, you can have comparable things such as symPyVariable1 and symPyVariable2.
| # Bring the split description in a deterministic order. | ||
| # NOTE: See note in `split_node()` why the sorting is done in this way. | ||
| # NOTE: The main benefit of bringing `split_description` into a deterministic | ||
| # order is that the output of this function is deterministic as well. I am | ||
| # not sure if there is any benefit beside that. | ||
| split_description = sorted(split_description, key=lambda split: str(split)) |
There was a problem hiding this comment.
see my other reply.
src/gt4py/next/program_processors/runners/dace/transformations/splitting_tools.py
Outdated
Show resolved
Hide resolved
…_more_deterministic
|
I have rerun the experiments again, because it was some time ago. 
It look like there is some degradation in the last stencil, but it is probably only a fluctuation. |
2 participants
Makes
gt_split_access_nodes()deterministic by processing it in the order ofAccessNodes.However, this means that the name of the data must be predictable and stable.
ToDo
gt_auto_optimization()gt_split_access_nodes(). C2SM/icon4py#937