Skip to content

GTC: Lower dimensional fields#327

Merged
DropD merged 51 commits intoGridTools:masterfrom
jdahm:gtc-field-dims
Mar 8, 2021
Merged

GTC: Lower dimensional fields#327
DropD merged 51 commits intoGridTools:masterfrom
jdahm:gtc-field-dims

Conversation

@jdahm
Copy link
Copy Markdown
Contributor

@jdahm jdahm commented Jan 31, 2021

Description

Implement lower dimensional fields in GTC. Higher dimensional fields will follow in a separate PR.

@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 5, 2021

I added some output checks to the generate test (had to change it a bit to not access undefined values). Surprisingly gtmc and gtc:gt:cpu_ifirst backends fail the tests locally (python 3.9), whereas on github they pass (python 3.8).

@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 5, 2021

bors try

bors bot added a commit that referenced this pull request Mar 5, 2021
@DropD DropD requested a review from tehrengruber March 5, 2021 13:33
@bors
Copy link
Copy Markdown

bors bot commented Mar 5, 2021

try

Build succeeded:

@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 8, 2021

bors try

bors bot added a commit that referenced this pull request Mar 8, 2021
@bors
Copy link
Copy Markdown

bors bot commented Mar 8, 2021

try

Build succeeded:

@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 8, 2021

bors try

bors bot added a commit that referenced this pull request Mar 8, 2021
Copy link
Copy Markdown
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

Looks good so far, just minor things I overlooked in my last review.

Comment thread src/gtc/utils.py


def dimension_flags_to_names(mask: Tuple[bool, bool, bool]) -> str:
labels = ["i", "j", "k"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of the LatLonGrid class, but for the sake of consistency should the names come from the same places as previously:
Domain.LatLonGrid().axes_names

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this PR is about introducing gtc support for lower dims I refuse to increase dependencies on the old toolchain here. Can be done somewhere else if justified.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, I have added a comment to remove the dependency on LatLonGrid above.

Comment thread src/gtc/oir.py Outdated
Comment thread tests/test_integration/test_code_generation.py Outdated
Comment thread src/gtc/common.py Outdated
@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 8, 2021

bors try

@bors
Copy link
Copy Markdown

bors bot commented Mar 8, 2021

try

Already running a review

Copy link
Copy Markdown
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

Last round I assume :-)

Comment thread src/gt4py/backend/gtc_backend/defir_to_gtir.py Outdated
Comment thread src/gtc/utils.py


def dimension_flags_to_names(mask: Tuple[bool, bool, bool]) -> str:
labels = ["i", "j", "k"]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, I have added a comment to remove the dependency on LatLonGrid above.

@bors
Copy link
Copy Markdown

bors bot commented Mar 8, 2021

try

Build succeeded:

@DropD
Copy link
Copy Markdown
Contributor

DropD commented Mar 8, 2021

bors try

bors bot added a commit that referenced this pull request Mar 8, 2021
@bors
Copy link
Copy Markdown

bors bot commented Mar 8, 2021

try

Build succeeded:

@DropD DropD merged commit faa5627 into GridTools:master Mar 8, 2021
@DropD DropD deleted the gtc-field-dims branch March 8, 2021 15:38
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.

4 participants