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

feat[next][dace]: Modify name of stride symbols #1548

Merged
merged 1 commit into from
Jun 6, 2024

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Jun 5, 2024

Remove unnecessary unique ID, which makes code generation easier for ICON-DSL bindings.

Remove unnecessary unique ID, which makes code generation easier for ICON-DSL bindings.
@edopao edopao marked this pull request as ready for review June 5, 2024 15:05
Copy link
Contributor

@philip-paul-mueller philip-paul-mueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two underscore have special meaning in DaCe, i.e. __return. I am pretty sure that there would not be a collision, but I suggest to you that you prefix the hole thing with something __gtit to make sure that it belongs to you.

Nevertheless, I will approve your PR, since it is a suggestion, and there is no need for a second round.

@edopao
Copy link
Contributor Author

edopao commented Jun 6, 2024

@philip-paul-mueller Thanks, I agree with your comment - I was also expecting it, since you are already commented this once. However, the name of the size argument a few lines above ("__{field_name}_size_{dim_index}") has to match the name of the scalar symbol generated by GT4Py. For the strides, there is no such argument, so I could follow your suggestion, but I prefer to follow the same name convention as the size argument. If I see that it stops working, for any reason, I will change it.

@edopao edopao merged commit f472c90 into GridTools:main Jun 6, 2024
31 checks passed
@edopao edopao deleted the dace-icon-dsl branch June 6, 2024 05:50
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.

2 participants