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

Draft: Slice reference output for unit tests to immitate stencil action #400

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

muellch
Copy link
Contributor

@muellch muellch commented Feb 29, 2024

No description provided.

@muellch muellch changed the title Slice reference output for unit tests to immitate stencil action Draft: Slice reference output for unit tests to immitate stencil action Feb 29, 2024
@muellch muellch marked this pull request as draft February 29, 2024 13:14
)

@pytest.fixture
def bounds(self, grid):
Copy link
Contributor

@samkellerhals samkellerhals Feb 29, 2024

Choose a reason for hiding this comment

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

Do you foresee the bounds being largely the same across most stencils? In that case it would be worth exploring whether you could define a "default" fixture in common, and then let the user overwrite it in each StencilTest

@@ -151,44 +152,49 @@ def allocate_data(backend, input_data):
}
return input_data

Bounds = namedtuple("Bounds", ["horizontal_start", "horizontal_end", "vertical_start", "vertical_end"])

@dataclass(frozen=True)
class Output:
Copy link
Contributor

@samkellerhals samkellerhals Feb 29, 2024

Choose a reason for hiding this comment

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

The Output class can now be deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

offset_provider=grid.offset_providers,
)

reference_data = allocate_data(backend, input_data)
Copy link
Contributor

@samkellerhals samkellerhals Feb 29, 2024

Choose a reason for hiding this comment

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

It may not be necessary to use allocate_data on the reference data, this is to construct gt4py fields and allocate them on the GPU. In this case a simple copy should probably be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no clue how this works, if the gt4py stuff is allocated on the GPU, then you have cupy fields instead of numpy fields?
Then let's say the computations happen on GPU, when we do the np.allclose() is the result field copied back to CPU before we do the comparison?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I think you can actually leave it as it is with the allocate_data, I missed that the data is being passed as numpy arrays here after all:

https://github.com/C2SM/icon4py/pull/400/files#diff-1a3cb4053c028972a019662fbfa45231da67d7ca1966526cc31725b998f416d9R176

self.PROGRAM.with_backend(backend)(
**input_data,
offset_provider=grid.offset_providers,
)
for out in self.OUTPUTS:
Copy link
Contributor

@samkellerhals samkellerhals Feb 29, 2024

Choose a reason for hiding this comment

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

Could you rename out to output_field_name or something like that for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

},
)

input_data = allocate_data(backend, input_data)
# To imitate the action of the gt4py stencil, we slice the reference output to only apply it in the given domain
Copy link
Contributor

@samkellerhals samkellerhals Feb 29, 2024

Choose a reason for hiding this comment

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

Could you add another sentence as to why we choose to imitate the action of the gt4py stencil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • launch jenkins spack

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

In case your change might affect downstream icon-exclaim, please consider running

  • launch jenkins icon

For more detailed information please look at CI in the EXCLAIM universe.

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.

2 participants