refactor[cartesian]: readability improvements in gtir -> oir conversion and other cleanups #1630

romanc · 2024-09-05T13:17:09Z

Description

Being new to the codebase and in preparation of the GT4Py-DaCe bridge refactor I was reading a lot of code. Some things that caught my eye are summarized in this PR. Bigger changes include

Moving daceir.py into the dace/ backend folder. This follows the convention of all other backends who have their IR inside the backend folder. (last commit)
Readability improvements in gtir_to_oir.py (the "Cleanup visit_*" commits): In my opinion, there's no point in shortening "statement" to "stmt". I wouldn't go as far as renaming the classes. For the renamed local variables, I think this adds a lot to readability.
visit_While in gtir_to_oir.py was having an extra mask statement around the while loop (in case a mask was defined. I inlined the potential mask with the condition of while loop.
In gtir_to_oir.py don't translate every body statement into a single MaskStmt. Instead, create one (or two incase of else) MaskStmt with multiple statements in the body (which is already typed as a list of statements).

Tested locally by running the test suite. All tests passing.

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:

The PR contains an updated version of the AUTHORS.md file adding the names of all the new contributors.

romanc · 2024-09-05T13:19:06Z

@egparedes is it okay to do such arguably stylistic/cosmetic changes?

/cc @twicki @FlorianDeconinck

romanc · 2024-09-05T13:28:16Z

Fixed the newly added type hints. Apparently TypeX | TypeY is only supported starting with python 3.10. Using Union[TypeX, TypeY] (like in the rest of the codebase) instead.

FlorianDeconinck

LGTM

egparedes · 2024-09-06T10:27:38Z

@egparedes is it okay to do such arguably stylistic/cosmetic changes?

Sure, if the changes make the code better and don't break the coding conventions I don't see why not.
By the way, there is no point in using explicit strings as type annotations when the module is using lazy annotation evaluations ( from __future__ import annotations). I know the unneeded stringification of the annotations was already there in many functions and modules and it's outside the scope of this PR to change it, so it's just to let you know...

I you update the PR to the current main, I can merge it afterwards.

romanc · 2024-09-06T11:08:11Z

By the way, there is no point in using explicit strings as type annotations when the module is using lazy annotation evaluations ( from __future__ import annotations). I know the unneeded stringification of the annotations was already there in many functions and modules and it's outside the scope of this PR to change it, so it's just to let you know...

Oh nice. My python is a bit outdated so I just copy-pasted. Now that I've read up on it, I agree, we should just use the new-style annotations. I'll create a separate PR as suggested.

I you update the PR to the current main, I can merge it afterwards.

Let's wait and see if @twicki has anything to say about this PR today. If there's no response by Monday, we can safely merge next week.

egparedes · 2024-09-06T14:30:32Z

cscs-ci run

romanc · 2024-09-06T15:30:49Z

cscs-ci run

@egparedes can you help me understand what the CI results from cscs mean? As far as I understand

it looks like a one of the next tests failed on changes in the cartesian/ folder. The test looks rather basic (some max operation) and values look rather weird

https://gitlab.com/cscs-ci/ci-testing/webhook-ci/mirrors/4455690602105886/4525297225819146/-/jobs/7764813849

Could it be that there's a race condition or reading uninitialized values from memory or something?

romanc · 2024-09-06T15:45:28Z

By the way, there is no point in using explicit strings as type annotations when the module is using lazy annotation evaluations ( from __future__ import annotations). I know the unneeded stringification of the annotations was already there in many functions and modules and it's outside the scope of this PR to change it, so it's just to let you know...

Oh nice. My python is a bit outdated so I just copy-pasted. Now that I've read up on it, I agree, we should just use the new-style annotations. I'll create a separate PR as suggested.

I've done this work in PR #1632. Let's merge PR #1632 first (I'll deal with any potential conflicts) because then it will be simpler to see which new "stringized" types I have introduced with this PR.

egparedes · 2024-09-09T11:04:18Z

@egparedes can you help me understand what the CI results from cscs mean? As far as I understand
I don't really understand it. Let's sync again this branch with main and try again.

romanc · 2024-09-09T14:54:47Z

@egparedes can you help me understand what the CI results from cscs mean? As far as I understand

I don't really understand it. Let's sync again this branch with main and try again.

sure, let's try and see - I'll rebase tomorrow once PR #1632 (new style type hints) is merged. That will anyway trigger updates in this branch too

romanc · 2024-09-10T08:09:06Z

okay so I rebased onto main and included changes from PR #1632 and now I get a CI error with jupyter notebooks. The jupyter notebook tests are fine on PR #1632 (and I guess on main too) and were fine before. The only new thing is commit d8dd0f9 fixing one more annotation. I guess that shouldn't break tests, right? Getting a bit confused here ...

In the past, the only way to create type hints for yet-to-be-defined types was to quote them. Since python 3.7 we can get rid of the quotes when using `annotations` from `__future__` instead. This is supposed to become the default in the future. This PR is a spin-off from #1630 (comment). Co-authored-by: Roman Cattaneo <>

egparedes · 2024-09-11T08:08:51Z

Let's solve the merge conflicts again first, and then I'll try to take a look if this PR still fails to pass the tests

Avoid the otherwise unused "result" variable and return directly.

No need for this `init()` function if we have a default value for the `parent` field.

No need to do this transformation manually. The same transformation is done inside `GridSubset.setInterval()` in case an `oir.Interval` is passed.

1. The extra mask statement around `oir.While` is unnecessary because the mask is already part of the loop's condition. 2. Cleanup naming to increase readability

All other backends (except the dace backend) have their IR in the backend-specific subfolder (e.g. `npir.py` inside the `numpy` folder). Let's do the same for the dace IR.

egparedes · 2024-09-11T08:15:10Z

cscs-ci run

romanc · 2024-09-11T08:49:40Z

Let's solve the merge conflicts again first, and then I'll try to take a look if this PR still fails to pass the tests

CI seems to be more happy today 🎉

This is a follow-up from PR #1630. It combines two things 1. In `oir_to_npir`, we fix conditional `while` loops in `numpy` backend. After PR 1630 these were stuck under certain conditions. Tests coverage extended. 2. In `gtir_to_oir`, we cleaned up the now unused `mask` parameter, which was pre-PR 1630 needed to pass down the mask information. With PR 1630 we actually removed the need for that parameter to be passed along because we properly nest the nested statements.

romanc force-pushed the romanc/cartesian-cleanup branch from 1f081ab to 6004dbd Compare September 5, 2024 13:26

FlorianDeconinck approved these changes Sep 5, 2024

View reviewed changes

FlorianDeconinck requested a review from twicki September 5, 2024 13:51

romanc mentioned this pull request Sep 6, 2024

refactor: use modern-style type hints #1632

Merged

3 tasks

romanc force-pushed the romanc/cartesian-cleanup branch from 6004dbd to d8dd0f9 Compare September 10, 2024 07:48

Roman Cattaneo added 12 commits September 11, 2024 10:10

No else after return in if statement

8c2a33e

Remove unused debug function

79d1465

Add more type hints

00d9128

Return directly

4e186b4

Avoid the otherwise unused "result" variable and return directly.

Cleanup DaCeIRBuilder.IterationContext

748dfe4

No need for this `init()` function if we have a default value for the `parent` field.

Cleanup GridSubset.set_interval usage

a05f440

No need to do this transformation manually. The same transformation is done inside `GridSubset.setInterval()` in case an `oir.Interval` is passed.

Cleanup visit_ParAssignStmt

bc78ab2

Cleanup visit_While

b0627ef

1. The extra mask statement around `oir.While` is unnecessary because the mask is already part of the loop's condition. 2. Cleanup naming to increase readability

Cleanup visit_HorizontalRestriction

4e11c0a

Clenaup visit_{if statements} and _VeritcalLoop

1297c4d

Move daceir into the dace folder

e539be2

All other backends (except the dace backend) have their IR in the backend-specific subfolder (e.g. `npir.py` inside the `numpy` folder). Let's do the same for the dace IR.

Use new-style type hints for AxisInterval

f49a04f

romanc force-pushed the romanc/cartesian-cleanup branch from d8dd0f9 to f49a04f Compare September 11, 2024 08:11

egparedes merged commit e0fb2a2 into GridTools:main Sep 11, 2024
39 checks passed

romanc deleted the romanc/cartesian-cleanup branch September 11, 2024 09:14

romanc mentioned this pull request Oct 25, 2024

fix[cartesian]: While loops inside conditions #1712

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor[cartesian]: readability improvements in gtir -> oir conversion and other cleanups #1630

refactor[cartesian]: readability improvements in gtir -> oir conversion and other cleanups #1630

romanc commented Sep 5, 2024 •

edited

Loading

romanc commented Sep 5, 2024

romanc commented Sep 5, 2024

FlorianDeconinck left a comment

egparedes commented Sep 6, 2024 •

edited

Loading

romanc commented Sep 6, 2024

egparedes commented Sep 6, 2024

romanc commented Sep 6, 2024

romanc commented Sep 6, 2024

egparedes commented Sep 9, 2024

romanc commented Sep 9, 2024

romanc commented Sep 10, 2024

egparedes commented Sep 11, 2024

egparedes commented Sep 11, 2024

romanc commented Sep 11, 2024

refactor[cartesian]: readability improvements in gtir -> oir conversion and other cleanups #1630

refactor[cartesian]: readability improvements in gtir -> oir conversion and other cleanups #1630

Conversation

romanc commented Sep 5, 2024 • edited Loading

Description

Requirements

romanc commented Sep 5, 2024

romanc commented Sep 5, 2024

FlorianDeconinck left a comment

Choose a reason for hiding this comment

egparedes commented Sep 6, 2024 • edited Loading

romanc commented Sep 6, 2024

egparedes commented Sep 6, 2024

romanc commented Sep 6, 2024

romanc commented Sep 6, 2024

egparedes commented Sep 9, 2024

romanc commented Sep 9, 2024

romanc commented Sep 10, 2024

egparedes commented Sep 11, 2024

egparedes commented Sep 11, 2024

romanc commented Sep 11, 2024

romanc commented Sep 5, 2024 •

edited

Loading

egparedes commented Sep 6, 2024 •

edited

Loading