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

refactor[cartesian]: gt4py/dace bridge cleanup #1895

Merged
merged 7 commits into from
Mar 4, 2025

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Mar 3, 2025

Description

In preparation for PR #1894, pull out some refactors and cleanups. Notable in this PR are the changes to src/gt4py/cartesian/gtc/dace/oir_to_dace.py

  • visit stencil.vertical_loops directly instead of calling generic_visit (simplification since there's nothing else to visit)
  • rename library nodes from f"{sdfg_name}_computation_{id(node)}" to f"{sdfg_name}_vloop_{counter}_{node.loop_order}_{id(node)}". This adds a bit more information (because sdfg_name is the same for all library nodes) and thus simplifies debugging workflows.

Related issue: GEOS-ESM/NDSL#53

Requirements

  • All fixes and/or new features come with corresponding tests.
    covered by existing test suite
  • Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder.
    N/A

romanc added 7 commits March 3, 2025 13:43
In the oir -> dace lowering (the one with the unexpanded library nodes,
don't use the generic visit function. Instead, visit the vertical loops
directly.
Add a bit more information to the library node name. This facilitates
debugging in that it is easier to associate the orgininal vertical loops
with the nodes.
- Import is unused
- Comments are redundant with doc strings
Copy link
Contributor

@twicki twicki left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Copy link
Contributor

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

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

LGTM

@romanc romanc merged commit c610561 into GridTools:main Mar 4, 2025
25 checks passed
@romanc romanc deleted the romanc/dace-bridge-cleanup branch March 4, 2025 16:18
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