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

Fix usage of as_offset #542

Merged
merged 5 commits into from
Sep 12, 2024
Merged

Fix usage of as_offset #542

merged 5 commits into from
Sep 12, 2024

Conversation

tehrengruber
Copy link
Contributor

@tehrengruber tehrengruber commented Sep 11, 2024

The lowering of as_offset in GT4Py was wrong, which was discovered and fixed recently in GridTools/gt4py#1484. Apparently we have some field operators in icon4py that just massaged the code in such a way that the wrong behavior in GT4Py was "canceled" out. However with the fix in GT4Py this broke and and the type inference on ITIR rightfully complained with an error similar to

E               File "/scratch/mch/jenkins/workspace/Spack/spack_PR_2/spack/opt/spack/linux-sles15-zen3/gcc-11.3.0/py-gt4py-1.0.3.9-cj7moh4ujoru35gc7l5ote23jqmtmbng/lib/python3.11/site-packages/gt4py/next/iterator/type_system/type_synthesizer.py", line 44, in <lambda>
E                 self.type_synthesizer = lambda *args, offset_provider: synthesizer(*args)
E                                                                        ^^^^^^^^^^^^^^^^^^
E               File "/scratch/mch/jenkins/workspace/Spack/spack_PR_2/spack/opt/spack/linux-sles15-zen3/gcc-11.3.0/py-gt4py-1.0.3.9-cj7moh4ujoru35gc7l5ote23jqmtmbng/lib/python3.11/site-packages/gt4py/next/iterator/type_system/type_synthesizer.py", line 352, in apply_shift
E                 assert found
E             AssertionError
E             
E           assert 1 == 0
E            +  where 1 = <Result AssertionError()>.exit_code

This PR is meant as a starting point to fix the remaining stencils (e.g. _truly_horizontal_diffusion_nabla_of_theta_over_steep_points looks like a candidate). Note that we can currently not detect these issues in the frontend as the experimental as_offset has a conceptual problem preventing this which can be understood from this snippet:

@field_operator
def foo(inp_field: Field[Vertex, K], index_field: Field[Vertex, K]):
    return inp_field(as_offset(KOff, index_field))

@field_operator
def foo(inp_field: Field[Vertex, K], index_field: Field[Edge, K]):
    # this passes the frontend type check, but is obviously wrong
    return inp_field(as_offset(KOff, index_field))

I very much suspect field operators that use values of the erroneous field operators, e.g. combined (formerly called fused) field operators, and shift those values have produced incorrect results in the past. For the person taking over this PR please be very careful and validate that my changes below.

@edopao edopao changed the title Fix compute_hydrostatic_correction_term Fix usage of as_offset Sep 11, 2024
@edopao
Copy link
Contributor

edopao commented Sep 11, 2024

cscs-ci run default

@edopao
Copy link
Contributor

edopao commented Sep 11, 2024

launch jenkins spack

Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

For more detailed information please look at CI in the EXCLAIM universe.

@edopao
Copy link
Contributor

edopao commented Sep 12, 2024

cscs-ci run default

1 similar comment
@halungge
Copy link
Contributor

cscs-ci run default

@edopao
Copy link
Contributor

edopao commented Sep 12, 2024

launch jenkins spack

@edopao edopao merged commit 8e825ae into main Sep 12, 2024
5 checks passed
@edopao edopao deleted the fix_wrong_as_offset_usage branch September 12, 2024 11:32
OngChia pushed a commit that referenced this pull request Jan 14, 2025
Fix usage of as_offset in following stencils:
* _compute_hydrostatic_correction_term
* _compute_horizontal_gradient_of_exner_pressure_for_multiple_levels
* _truly_horizontal_diffusion_nabla_of_theta_over_steep_points

---------

Co-authored-by: Edoardo Paone <[email protected]>
Co-authored-by: Magdalena Luz <[email protected]>
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