Skip to content

Clean up gtc:numpy backend and fix issues#628

Merged
jdahm merged 14 commits intoGridTools:masterfrom
jdahm:numpy-masks-on-scalars
Feb 18, 2022
Merged

Clean up gtc:numpy backend and fix issues#628
jdahm merged 14 commits intoGridTools:masterfrom
jdahm:numpy-masks-on-scalars

Conversation

@jdahm
Copy link
Copy Markdown
Contributor

@jdahm jdahm commented Jan 25, 2022

Description

Rewrites the numpy backend to resolve a number of outstanding bugs. The biggest changes are:

  • Uses the SymbolName, SymbolRef utilities of eve.
  • Tracks scalar vs vector expr type and applies broadcasts, serial casts, and vector casts.
  • Calls oir utility to compute extents when lowering to npir, and stores this in the npir.
  • Enables the gtc:numpy backend for integration tests that were skipping it.
  • Add GT_CACHE_PYTEST_DIR env var to control pytest dir. This is useful for debugging, because breakpoints can be set in the generated Python code.

Resolves #632.
Resolves #621.

To do:

  • Add back code-generation unit tests.
  • Add back oir_to_npir tests.
  • Decide where to move StencilExtentComputer for cleaner dependency chain.

@jdahm jdahm requested a review from DropD January 25, 2022 20:57
@jdahm jdahm marked this pull request as draft January 26, 2022 21:22
@jdahm jdahm removed the request for review from DropD January 28, 2022 06:25
@jdahm jdahm changed the title Add mask_acc to NamedScalars and VectorTemps Cleanup gtc:numpy backend Feb 2, 2022
@jdahm jdahm changed the title Cleanup gtc:numpy backend Cleanup gtc:numpy backend and fix issues Feb 2, 2022
@jdahm jdahm marked this pull request as ready for review February 4, 2022 01:04
@jdahm jdahm requested a review from havogt February 4, 2022 01:04
@jdahm jdahm changed the title Cleanup gtc:numpy backend and fix issues Clean up gtc:numpy backend and fix issues Feb 4, 2022
@jdahm
Copy link
Copy Markdown
Contributor Author

jdahm commented Feb 7, 2022

bors try

jdahm added a commit to eddie-c-davis/gt4py that referenced this pull request Feb 7, 2022
bors Bot added a commit that referenced this pull request Feb 8, 2022
@jdahm
Copy link
Copy Markdown
Contributor Author

jdahm commented Feb 8, 2022

The daint tests passed, there was just a problem propagating that back to GitHub here.

@bors
Copy link
Copy Markdown

bors Bot commented Feb 9, 2022

try

Timed out.

Copy link
Copy Markdown
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 comments but it looks good to me. Additionally, I think someone else with a deeper knowledge of the gtc toolchain should review the PR.

Comment thread src/gtc/numpy/npir.py Outdated
Comment thread src/gtc/numpy/npir.py Outdated
Comment thread src/gtc/numpy/npir.py Outdated
Comment thread src/gtc/numpy/npir.py Outdated
Comment thread src/gtc/numpy/npir.py Outdated
Comment thread src/gtc/numpy/npir_codegen.py Outdated
Comment thread src/gtc/numpy/npir_codegen.py
Comment thread src/gtc/numpy/npir_codegen.py Outdated
Comment thread src/gtc/numpy/npir_codegen.py Outdated
Comment thread src/gtc/numpy/npir_codegen.py Outdated
Copy link
Copy Markdown
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.

Almost ready from my side, just a couple of small issues.

Comment thread src/gtc/common.py Outdated
Comment thread src/gtc/common.py Outdated
Comment thread src/gtc/numpy/npir.py Outdated
@jdahm
Copy link
Copy Markdown
Contributor Author

jdahm commented Feb 17, 2022

bors try

bors Bot added a commit that referenced this pull request Feb 17, 2022
@bors
Copy link
Copy Markdown

bors Bot commented Feb 17, 2022

try

Build succeeded:

@jdahm jdahm requested a review from egparedes February 18, 2022 04:03
Copy link
Copy Markdown
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 from my side. It would be great if someone else with a deeper knowledge of GTC would have time to review it today, but it's not a strict requirement so I'm approving the PR. @jdahm can merge in the evening if there are no comments from anyone else.

@jdahm
Copy link
Copy Markdown
Contributor Author

jdahm commented Feb 18, 2022

bors try

bors Bot added a commit that referenced this pull request Feb 18, 2022
@bors
Copy link
Copy Markdown

bors Bot commented Feb 18, 2022

try

Build succeeded:

@jdahm jdahm merged commit 7704eba into GridTools:master Feb 18, 2022
@jdahm jdahm deleted the numpy-masks-on-scalars branch February 18, 2022 20:39
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.

Incorrect assumptions in gtc:numpy backend gtc:numpy backend fails on TestNon3DFields

2 participants