Skip to content

Conversation

@twicki
Copy link
Collaborator

@twicki twicki commented Sep 2, 2025

WIP for dace orchestration

Copy link
Collaborator

@romanc romanc left a comment

Choose a reason for hiding this comment

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

I'd keep the current function names. Imo, the numpy_ prefix doesn't help understanding the code.


def numpy_corner_y(self, d2, nord_data: Quantity):
for k in range(nord_data.shape[0]):
if nord_data.data[k] > 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this now missing the previous comparison if nord > current_nord ? While the first two invocations hard-code current_node=0, the last two are run in a loop with current_nord between 0 and max(nord), which should - unless all nord values are the same - filter some edge computations, no?

def numpy_corner_y(self, d2, nord_data: Quantity):
for k in range(nord_data.shape[0]):
if nord_data.data[k] > 0:
d2[0, 0, k] = d2[5, 0, k]
Copy link

Choose a reason for hiding this comment

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

Forgive my ignorance, but is there a way that this could be written as a loop as in copy_corners() rather than explicitly writing out every element?

Copy link
Collaborator

@FlorianDeconinck FlorianDeconinck Sep 3, 2025

Choose a reason for hiding this comment

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

There is. This re-write is due to ongoing performance engineering work on CPU - it's not necessarily going to stay that way in the long run.

It's also a studying case for us of a few things:

  • a code that triggers heavy micro-op reject in stencils mode leading to poor performance
  • a case of code that needs to "escape" the SDFG and remain in the full program optimizer purview

@twicki twicki closed this Sep 24, 2025
@romanc
Copy link
Collaborator

romanc commented Sep 26, 2025

Superseded by NOAA-GFDL/NDSL#240 and PR #87.

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