Skip to content

GDP-2: Regions#24

Merged
egparedes merged 72 commits intoGridTools:masterfrom
ai2cm:gdp2
Mar 2, 2021
Merged

GDP-2: Regions#24
egparedes merged 72 commits intoGridTools:masterfrom
ai2cm:gdp2

Conversation

@jdahm
Copy link
Contributor

@jdahm jdahm commented Apr 23, 2020

Discussion about GDP-2. Credit goes to @rheacangeo who pushed this forward.

@jdahm jdahm closed this Apr 23, 2020
jdahm and others added 15 commits April 23, 2020 09:30
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
…Modeling/gt4py into edges_and_corners_proposal
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
Co-Authored-By: Jeremy McGibbon <jeremym@vulcan.com>
@jdahm
Copy link
Contributor Author

jdahm commented Nov 19, 2020

I will amend this to reflect the current regional computation concept.

@jdahm jdahm marked this pull request as ready for review November 19, 2020 19:00
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.

I left some comments about extending a bit more the examples and the explanations and a suggestion to improve the syntax.

@jdahm jdahm requested a review from egparedes January 5, 2021 03:13
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.

I have some minor comments to clarify some ambiguous points, but overall I think now the definition of the feature is clear. I would also like that someone else with experience in the GTScript parallel model (or the GridTools parallel model) reviewed the proposal to look for possible ambiguities in a C++ implementation. @havogt ?

Axis Offsets therefore interally hold another offset, which can be set by adding or subtracting from the subscript.
For example ``I[0] - 2`` is itself an Axis Offsets that refers to 2 points before the start of the compute domain.

Axis Offsets may be assigned to variables in Python and/or used as externals in GT4Py in order to define ``region``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that an AxisOffset object can be created and manipulated in Python or that AxisOffset indices can be Python variables? It's not 100% clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both. They are a proper class with __add__ __sub__, and the r-variants.

@jdahm jdahm requested a review from egparedes January 7, 2021 20:43
@jdahm
Copy link
Contributor Author

jdahm commented Feb 1, 2021

@egparedes @mbianco Is this ready to be accepted then?

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.

Last two minor changes to clarify and this is finally ready to be merged for me ASAP.

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.

@mbianco @havogt @tehrengruber @twicki @jdahm @ofuhrer, in my opinion the GDP is mature and stable enough to be merged. If there are not other comments or issues from anyone else, I propose to merge it early next week.

@jdahm
Copy link
Contributor Author

jdahm commented Mar 2, 2021

I am OK with merging this!

@egparedes egparedes merged commit 050773c into GridTools:master Mar 2, 2021
@jdahm jdahm deleted the gdp2 branch December 21, 2021 23:32
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.

5 participants