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]: Introduce FunctionField #1328

Open
wants to merge 67 commits into
base: main
Choose a base branch
from

Conversation

samkellerhals
Copy link
Contributor

@samkellerhals samkellerhals commented Aug 23, 2023

Description

Introduces a FunctionField which can also be used to make constant fields.

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

If this PR contains code authored by new contributors please make sure:

  • All the authors are covered by a valid contributor assignment agreement provided to ETH Zurich and signed by the employer if needed.
  • The PR contains an updated version of the AUTHORS.md file adding the names of all the new contributors.

@samkellerhals samkellerhals changed the title Introduce ConstantField feat[next]: Introduce ConstantField Aug 23, 2023
src/gt4py/next/common.py Show resolved Hide resolved
src/gt4py/next/embedded/function_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/function_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/function_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/common.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/function_field.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/function_field.py Outdated Show resolved Hide resolved
@samkellerhals
Copy link
Contributor Author

@havogt Should be ready for another review now. Only thing I am still missing is to make mypy happy on the three issues below:

src/gt4py/next/embedded/function_field.py:222:12: error: Cannot instantiate abstract class "FunctionField" with abstract attributes "__gt_builtin_func__", "__gt_dims__", "__gt_origin__" and "dtype"  [abstract]
src/gt4py/next/embedded/function_field.py:232:12: error: Cannot instantiate abstract class "FunctionField" with abstract attributes "__gt_builtin_func__", "__gt_dims__", "__gt_origin__" and "dtype"  [abstract]
src/gt4py/next/embedded/function_field.py:235:58: error: Argument 2 to "register_builtin_func" of "FieldBuiltinFuncRegistry" has incompatible type "Callable[[FunctionField[Any, Any], tuple[Dimension, ...]], FunctionField[Any, Any]]"; expected "Callable[[Field[Any, Any] | gt4py_defs.ScalarT, tuple[Dimension, ...]], Field[Any, Any] | Any] | None"  [arg-type]

Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

Sorry, had these pending comments, some of them should be obsolete now.

tests/next_tests/unit_tests/embedded_tests/test_common.py Outdated Show resolved Hide resolved
tests/next_tests/unit_tests/embedded_tests/test_common.py Outdated Show resolved Hide resolved
tests/next_tests/unit_tests/embedded_tests/test_common.py Outdated Show resolved Hide resolved
@@ -0,0 +1,298 @@
# GT4Py - GridTools Framework
Copy link
Contributor

Choose a reason for hiding this comment

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

As you already pointed out, there are some tests that should be run on all fields (e.g. all fields need to support all operators and fbuiltins). I think it makes sense to try to extract these into a test that works universally.
I didn't think about the problem, but maybe just compare a numpy reference against field.ndarray could be good enough.

Of course, there are also other tests that should stay in the concrete field test file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried cleaning the tests up a bit by introducing a test in nd_array_field which tests the unary builtins for all of the field types (BaseNdarrayField and FunctionField). I made sure to share the operator fixtures across the ndarray and function field tests to ensure that the same operators are tested in both instances, see 2a18ccc

Other than that I don't know how much value there is to refactor the code even further, do you want to also have some sort of global test for all of the operators?

src/gt4py/next/embedded/exceptions.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/function_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/function_field.py Outdated Show resolved Hide resolved
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.

None yet

2 participants