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] Enable embedded field view in ffront_tests #1361

Merged
merged 19 commits into from
Nov 17, 2023

Conversation

havogt
Copy link
Contributor

@havogt havogt commented Nov 16, 2023

Enables field view in ffront_tests

New exclusion markers for some cases

  • cartesian and unstructured shifts
  • scan
  • check for a very concrete error message in parsing: we should match this later in embedded

Adds the following features to embedded:

  • support for scalar broadcast, astype, binary functions
  • adds __ne__ and __eq__ to Field

TODOs:

  • full comparison operators for UnitRange
  • full comparison operators for Fields

src/gt4py/next/allocators.py Outdated Show resolved Hide resolved
@havogt havogt requested a review from egparedes November 17, 2023 07:14
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/common.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
@@ -705,13 +706,33 @@ def __call__(
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)
self.as_program(arg_types, kwarg_types)(
*args, out, offset_provider=offset_provider, **kwargs
)
return out

Just noticed this bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should field_operator as program return the out param? Maybe yes...

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say no as programs don't return anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

than it was correct, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the program does not return something, but the field operator which we are calling does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, we return self.as_program()() i.e. whatever program does which is currently nothing therefore still correct and in case (for whatever reason) program() returns something, we will do the right thing implicitly.

src/gt4py/next/ffront/decorator.py Show resolved Hide resolved
) -> common.Field | core_defs.ScalarT:
assert core_defs.is_scalar_type(lhs)
assert core_defs.is_scalar_type(rhs)
return BINARY_MATH_NUMBER_BUILTIN_TO_PYTHON_SCALAR_FUNCTION[name](lhs, rhs) # type: ignore[operator] # Cannot call function of unknown type
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return BINARY_MATH_NUMBER_BUILTIN_TO_PYTHON_SCALAR_FUNCTION[name](lhs, rhs) # type: ignore[operator] # Cannot call function of unknown type
return getattr(np, name)(lhs, rhs)

Works just fine for scalars and the dict (with the clumsy name) is not needed in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that's not what it does. it maps to python builtins, but thanks to our clever naming, they have different name...

Copy link
Contributor

@tehrengruber tehrengruber Nov 17, 2023

Choose a reason for hiding this comment

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

I feel I'm missing something obvious. np.minimum is an extension of the functionality of min, so I don't understand your first sentence. Unless my assumption is wrong our naming is indeed clever as we chose the numpy name for all builtins and not sometimes the python ones in case they exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah now, I get it, you want to do the scalar functions using numpy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/gt4py/next/ffront/fbuiltins.py Outdated Show resolved Hide resolved
@havogt havogt requested a review from tehrengruber November 17, 2023 14:30
@havogt havogt merged commit ecd0b68 into GridTools:main Nov 17, 2023
33 checks passed
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