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

Non-ideal fix for unreachable scalar variables #69

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

fmalatino
Copy link
Contributor

@fmalatino fmalatino commented Sep 4, 2024

Description
As authored by @FlorianDeconinck. Current hotfix for scalar variables which are unreachable during DaCe orchestration procedure

(HOT)Fixes # 42
Hot fix until DaCe, GT4Py bridge refactor

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included

oelbert
oelbert previously approved these changes Sep 4, 2024
Copy link
Collaborator

@oelbert oelbert left a comment

Choose a reason for hiding this comment

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

🙃

twicki
twicki previously approved these changes Sep 5, 2024
Copy link
Collaborator

@twicki twicki left a comment

Choose a reason for hiding this comment

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

We take what we need. Can we have reverting this reflected in an issue such that we come back to it once we can?

@FlorianDeconinck
Copy link
Collaborator

Logging the full story + examples + patch code fix that partially fixes things here: #70

@bensonr
Copy link

bensonr commented Sep 5, 2024

Do we need the same construct for y_out/y_in?

@FlorianDeconinck
Copy link
Collaborator

Do we need the same construct for y_out/y_in?

No, we are just forcing the stencil to not become empty when all the horizontal region are being culled. Empty stencil is not an issue but we have a signature/argument passing problem that show up when it happens.

I asked Frank to change it to x_out=x_out which is nonsensical but keeps the stencil from being entirely empty.

@bensonr
Copy link

bensonr commented Sep 5, 2024

The acoef=mysign was put in to fix a similar issue with scalar variable removal. Are these separate, but related issues?

@FlorianDeconinck
Copy link
Collaborator

The acoef=mysign was put in to fix a similar issue with scalar variable removal. Are these separate, but related issues?

Exactly... It all boils down to: we have a bridle way to link signature as described by GT4Py, real call signature in code and SDFG expectation for data going in and out. #70 has a series of 3 tests that encompasses all the issues and some code that fix some of it (but not all).

I believe the right way to deal with this is a good top to bottom clean up of the call structure, simplifying some optimization that are micro-opt with little to no performance improvement and complexify the entire system

@xyuan
Copy link
Collaborator

xyuan commented Sep 5, 2024 via email

@fmalatino fmalatino dismissed stale reviews from twicki and oelbert via e3cca7c September 5, 2024 18:13
@FlorianDeconinck
Copy link
Collaborator

FlorianDeconinck commented Sep 6, 2024

Withholding merge on this as I have implemented a proper fix gt4py side.

Working branch: https://github.com/FlorianDeconinck/gt4py/tree/cartesian/fix/missing_parameter

@FlorianDeconinck
Copy link
Collaborator

Seems like the proper fix is not quite there. We will be merging this to unblock release.

@FlorianDeconinck FlorianDeconinck merged commit 1773365 into NOAA-GFDL:develop Sep 9, 2024
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.

6 participants