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.

Sign up for GitHub

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

Jump to bottom

cartesian: dace backends missing support for casting in variable k offsets #1881

Open
romanc opened this issue Feb 20, 2025 · 0 comments
Open

cartesian: dace backends missing support for casting in variable k offsets #1881

romanc opened this issue Feb 20, 2025 · 0 comments
Labels
gt4py.cartesian Issues concerning the current version with support only for cartesian grids. module: backend Related to analysis/backend subpackages triage: bug Something isn't working

Comments

@romanc
Copy link
Contributor

romanc commented Feb 20, 2025

Description

DaCe backends are currently missing support for casting in VariableKOffset. The issue comes from the fact that we "sympify" index computations in the _visit_offsets() function of tasklet_codegen.

gt4py/src/gt4py/cartesian/gtc/dace/expansion/tasklet_codegen.py

Lines 46 to 49 in 1a46fb0

sym_offsets = [
dace.symbolic.pystr_to_symbolic(self.visit(off, **kwargs))
for off in (node.to_dict()["i"], node.to_dict()["j"], node.k)
]

However, before we "sympify", casts are already translated to dace types, e.g. dace.int64 and then this isn't plain python anymore and fails sympy's python parser.

PR ... puts a mitigation in place to catch casts in indices of type VariableKOffset in the DaCe backends. The PR also adds a codgen test which

  • uses xfail for all DaCe backends
  • succeeds for all other backends

That behavior can be simplified once DaCe backends also support casting in variable k offset expressions.
@romanc romanc added gt4py.cartesian Issues concerning the current version with support only for cartesian grids. module: backend Related to analysis/backend subpackages triage: bug Something isn't working labels Feb 20, 2025
@romanc romanc mentioned this issue Feb 20, 2025
Merged
2 tasks
FlorianDeconinck added a commit that referenced this issue Feb 21, 2025
@romanc @FlorianDeconinck
fix[cartesian, dace]: warn about missing support for casting in varia…
1984691 
…ble k offsets (#1882)

<!--
Delete this comment and add a proper description of the changes
contained in this PR. The text here will be used in the commit message
since the approved PRs are always squash-merged. The preferred format
is:

- PR Title: <type>[<scope>]: <one-line-summary>

    <type>:
- build: Changes that affect the build system or external dependencies
        - ci: Changes to our CI configuration files and scripts
        - docs: Documentation only changes
        - feat: A new feature
        - fix: A bug fix
        - perf: A code change that improves performance
- refactor: A code change that neither fixes a bug nor adds a feature
        - style: Changes that do not affect the meaning of the code
        - test: Adding missing tests or correcting existing tests

    <scope>: cartesian | eve | next | storage
    # ONLY if changes are limited to a specific subsystem

- PR Description:

Description of the main changes with links to appropriate
issues/documents/references/...
-->

## Description

We figured that DaCe backends are currently missing support for casting
in variable k offsets. This PR

- adds a codegen test with a cast in a variable k offset
- adds a node validator for the DaCe backends complaining about missing
for support.
- adds an `xfail` test for the node validator

This should be fixed down the road. Here's the issue
#1881 to keep track.

The PR also has two smaller and unrelated commits

- 741c448 increases test coverage with
another codgen test that has a couple of read after write access
patterns which were breaking the "new bridge" (see
GEOS-ESM/NDSL#53).
- e98ddc5 just forwards all keyword
arguments when visiting offsets. I don't think this was a problem until
now, but it's best practice to forward everything.

## Requirements

- [x] All fixes and/or new features come with corresponding tests.
- [ ] Important design decisions have been documented in the appropriate
ADR inside the [docs/development/ADRs/](docs/development/ADRs/Index.md)
folder.
  N/A

---------

Co-authored-by: Roman Cattaneo <[email protected]>
Co-authored-by: Florian Deconinck <[email protected]>
@romanc romanc mentioned this issue Feb 25, 2025
Merged
2 tasks
havogt pushed a commit that referenced this issue Feb 27, 2025
@romanc
test[cartesian]: conditional xfail instead of two tests (#1885)
847d8ab 
Follow-up from PR #1882 to merge
two test cases which only differed in the expected outcome. With this
PR, we'll have one single test case with a conditional `xfail` as
@havogt proposed in the GridTools slack channel.

Related issue: #1881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gt4py.cartesian Issues concerning the current version with support only for cartesian grids. module: backend Related to analysis/backend subpackages triage: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@romanc