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

Extend ITIR type inference to collect the types of all subexpressions #1155

Merged
merged 23 commits into from
Feb 24, 2023

Conversation

tehrengruber
Copy link
Contributor

@tehrengruber tehrengruber commented Feb 7, 2023

Type inference on ITIR was previously only returning the type of a single expression. This PR introduces extends the inference to collect the type of all subexpressions and introduces a new function _infer_all returning a mapping from the (Python) id of a node to its type. As we start using this function we will make it public.

@tehrengruber tehrengruber changed the base branch from main to functional February 7, 2023 14:39
@tehrengruber tehrengruber changed the base branch from functional to main February 14, 2023 16:41
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 good. Besides a couple of missing type hints, the rest of my comments are just questions and optional suggestions.

src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.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.

Just two comments from my side (and the failing tests) and it will be ready to go.

Comment on lines 401 to 402
collected_types Mapping from the (Python) id of a node to its type.
constraints Set of constraints, where a constraint is a pair of types that need to
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the correct Google style format for the docstrings (as required by sphix-napoleon: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html#example-google)

Suggested change
collected_types Mapping from the (Python) id of a node to its type.
constraints Set of constraints, where a constraint is a pair of types that need to
collected_types: Mapping from the (Python) id of a node to its type.
constraints: Set of constraints, where a constraint is a pair of types that need to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. This should be adopted in the https://github.com/GridTools/gt4py/blob/main/CODING_GUIDELINES.rst at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was already there in functional (https://github.com/GridTools/gt4py/blob/functional/CODING_GUIDELINES.md#docstrings), but it got lost during the merge. I will update soon the documents in master with the versions from the other branch.

src/gt4py/next/iterator/type_inference.py Outdated Show resolved Hide resolved
src/gt4py/next/iterator/type_inference.py Show resolved Hide resolved
@tehrengruber
Copy link
Contributor Author

Blocked by #1182

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 good. Don't forget to add a proper commit message.

@tehrengruber tehrengruber changed the title ITIR typing all nodes Extend type inference to collect the types of all subexpressions of a tree. Feb 24, 2023
@tehrengruber tehrengruber changed the title Extend type inference to collect the types of all subexpressions of a tree. Extend ITIR type inference to collect the types of all subexpressions Feb 24, 2023
@tehrengruber tehrengruber merged commit 481ecca into GridTools:main Feb 24, 2023
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