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]: Embedded support for skip value connectivities #1441

Merged
merged 24 commits into from
Feb 23, 2024

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Feb 1, 2024

  • Introduces common._DEFAULT_SKIP_VALUE = -1, currently we don't support execution with skip_values which are different than that default.
  • inverse_image works by ignoring the skip_value for the inverse image calculation. current result is the smallest hypercube that contains all non skip values that are requested.

Additional changes:

  • Adds a ffront test for the fvm nabla example
  • Fixes a lowering bug from past to itir, where domain didn't respect order of horizontal and vertical dimension

@havogt havogt requested a review from egparedes February 7, 2024 08:48
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

It looks correct to me. My comments are mostly about style, and some questions and typo fixes.

src/gt4py/next/ffront/decorator.py Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Show resolved Hide resolved
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

Last questions and comments...

Comment on lines 319 to +320
_codomain: common.DimT
_skip_value: Optional[core_defs.IntegralScalar]
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope of this PR but just to not forget: it doesn't make sense to use a property for plain attribute access just because a property was defined in a parent class. We could add a simple utility to eve to disable parent properties by overwriting them with a non-data descriptor like this:

def disable_property(default=...) -> functools.cached_property:
    return functools.cached_property(lambda _: default)

then this should work:

Suggested change
_codomain: common.DimT
_skip_value: Optional[core_defs.IntegralScalar]
codomain: common.DimT = disable_property()
skip_value: Optional[core_defs.IntegralScalar] = disable_property()

Inside a dataclass, it would also work to define a field with a default like this:

    codomain: common.DimT = dataclasses.field(default=...)

although this would change the semantics by making codomain optional, where disable_property wouldn't affect the behavior of the dataclass at all.

src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
Copy link
Contributor

@egparedes egparedes left a comment

Choose a reason for hiding this comment

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

ILGTM.

@havogt havogt merged commit 117de0a into GridTools:main Feb 23, 2024
31 checks passed
@havogt havogt deleted the embedded_skip_value_connectivity branch February 23, 2024 15:14
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