[MPS] Add portable grid_sampler_2d implementation + tests #10561

DenisVieriu97 · 2025-04-29T21:17:17Z

Upstream grid_sampler_2d (cpu implementation) and the tests implemented by @versi379

Summary of changes:

Add grid_sampler_2d implementation
Add tests for grid_sampler_2d tests

cc @cccclai , @shoumikhin

pytorch-bot · 2025-04-29T21:17:20Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10561

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

Multiple CI trunk failures after landing https://github.com/pytorch/pytorch/pull/161002

✅ No Failures

As of commit b4a4e04 with merge base b4e1145 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

tarun292 · 2025-05-02T22:14:08Z

@cccclai @manuelcandales bumping up for a review.

cccclai · 2025-05-05T16:41:18Z

It's portable op implementation, can @manuelcandales or @swolchok take a look?

swolchok

I'm not familiar with this operator and its mathematical function, but here are some general C++ comments

swolchok · 2025-05-05T17:11:47Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+namespace torch {
+namespace executor {
+namespace native {


nit: we have C++17, new code should use nested namespaces (namespace torch::executor::native {)

swolchok · 2025-05-05T17:11:53Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+using Tensor = exec_aten::Tensor;
+using ScalarType = executorch::aten::ScalarType;
+using SizesType = executorch::aten::SizesType;


swolchok · 2025-05-05T17:12:11Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+using SizesType = executorch::aten::SizesType;
+
+// Transform normalized coordinates to pixel space
+inline float unnormalize_coord(float coord, int64_t size, bool align_corners) {


for internal functions like this one: remove inline, put in anonymous namespace

swolchok · 2025-05-05T17:13:26Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+}
+
+// Compute source index and interpolation weight
+inline std::pair<int64_t, float>


ditto inline

swolchok · 2025-05-05T17:17:26Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+  int64_t abs_coord = std::abs(coord);
+  abs_coord = abs_coord % double_size;


super nit: I see a couple places in this PR where you could be terser by performing more than one operation per statement. for example, here would be better as int64_t abs_coord = std::abs(coord) % double_size;

swolchok · 2025-05-05T17:27:14Z

kernels/test/op_grid_sampler_2d_test.cpp

+
+    // Create a 1x1x4x4 input tensor
+    const std::vector<int32_t> input_sizes = {1, 1, 4, 4};
+    std::vector<float> input_data = {


nit: you could use std::iota to make this a lot shorter
https://en.cppreference.com/w/cpp/algorithm/iota

swolchok · 2025-05-05T17:27:42Z

kernels/test/op_grid_sampler_2d_test.cpp

+        align_corners,
+        out);
+
+    // Check non-zero


this comment just repeats the code, delete (and similar comments throughout that repeat the code)

swolchok · 2025-05-05T17:28:06Z

kernels/test/op_grid_sampler_2d_test.cpp

+
+    // Create a 1x1x4x4 input tensor
+    const std::vector<int32_t> input_sizes = {1, 1, 4, 4};
+    std::vector<float> input_data = {


swolchok · 2025-05-05T17:28:41Z

shim_et/xplat/executorch/kernels/portable/op_registration_util.bzl

@@ -1330,4 +1337,4 @@ def portable_source_list():

 def portable_header_list():
    """All the header file names from //executorch/kernels/portable/cpu/"""
-    return ["selective_build.h", "scalar_utils.h", "math_constants.h", "vec_ops.h"]
+    return ["selective_build.h", "scalar_utils.h", "math_constants.h", "vec_ops.h"]


not 100% sure, but it looks like github is saying you removed the end-of-file newline? if so, please put it back

swolchok · 2025-05-05T17:29:06Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+      (out.size(0) == N && out.size(1) == C && out.size(2) == out_H &&
+       out.size(3) == out_W),


shouldn't we be attempting to resize output to the desired size so that we can handle dynamic shapes smoothly? @manuelcandales

Yes.
@DenisVieriu97 please follow this pattern

manuelcandales · 2025-05-07T00:23:47Z

kernels/test/targets.bzl

@@ -335,4 +336,4 @@ def define_common_targets():
    _common_op_test("op_view_as_real_copy_test", ["aten", "portable"])
    _common_op_test("op_view_copy_test", ["aten", "portable"])
    _common_op_test("op_where_test", ["aten", "portable"])
-    _common_op_test("op_zeros_test", ["aten", "portable"])
+    _common_op_test("op_zeros_test", ["aten", "portable"])


manuelcandales · 2025-05-07T00:35:18Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+  const int64_t out_W = grid.size(2);
+
+  // Check for 4D input and grid
+  ET_KERNEL_CHECK(ctx, (input.dim() == 4), InvalidArgument, out);


@DenisVieriu97 do you mind refactoring all of the InvalidArgument checks into a helper function dedicated to check the arguments, which returns True only if all checks pass, and logs when a check doesn't pass. Here is an example
and then you would be calling
ET_KERNEL_CHECK(ctx, check_grid_sampler_2d_args(...) == 4), InvalidArgument, out);

manuelcandales · 2025-05-07T00:38:34Z

kernels/portable/cpu/op_grid_sampler_2d.cpp

+
+// Process grid sampling with specified input, grid, and modes
+template <typename T>
+void grid_sampler_2d_impl(


This implementation requires default dim order. Add a check in your check_args function, to check that the dim order is the default.

…support

github-actions · 2025-08-27T19:45:49Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

DenisVieriu97 requested review from manuelcandales and swolchok as code owners April 29, 2025 21:17

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 29, 2025

shoumikhin added topic: not user facing ciflow/trunk labels Apr 29, 2025

DenisVieriu97 force-pushed the dev/denis/upstream_giovanni_grid_sampler_2d_support branch from 8f26df6 to 682b539 Compare April 29, 2025 21:58

pytorch-bot bot removed the ciflow/trunk label Apr 29, 2025

Giovanni Versiglioni added 4 commits April 30, 2025 21:17

Add portable grid_sampler_2d implementation + tests

650fa2f

executorch clang-format + review updates

ad814c0

review updates: newlines

96f32ef

review updates: newlines fix

4450646

DenisVieriu97 force-pushed the dev/denis/upstream_giovanni_grid_sampler_2d_support branch from 682b539 to 4450646 Compare May 1, 2025 04:17

swolchok requested changes May 5, 2025

View reviewed changes

manuelcandales reviewed May 7, 2025

View reviewed changes

Merge branch 'main' into dev/denis/upstream_giovanni_grid_sampler_2d_…

b4a4e04

…support

manuelcandales requested review from larryliu0820 and kirklandsign as code owners August 27, 2025 19:45

manuelcandales mentioned this pull request Aug 27, 2025

[ET][Portable] Add op grid_sampler_2d #13734

Open

		int64_t abs_coord = std::abs(coord);
		abs_coord = abs_coord % double_size;

		(out.size(0) == N && out.size(1) == C && out.size(2) == out_H &&
		out.size(3) == out_W),

[MPS] Add portable grid_sampler_2d implementation + tests #10561

Are you sure you want to change the base?

[MPS] Add portable grid_sampler_2d implementation + tests #10561

Uh oh!

Conversation

DenisVieriu97 commented Apr 29, 2025

Uh oh!

pytorch-bot bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10561

❗ 1 Active SEVs

✅ No Failures

Uh oh!

tarun292 commented May 2, 2025

Uh oh!

cccclai commented May 5, 2025

Uh oh!

swolchok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuelcandales May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 27, 2025

This PR needs a release notes: label

Uh oh!

Uh oh!

pytorch-bot bot commented Apr 29, 2025 •

edited

Loading

manuelcandales May 7, 2025 •

edited

Loading

This PR needs a `release notes:` label