Roialign fix and half_pixel mode support #3482

bpickrel · 2024-09-26T15:46:08Z

Fix bugs in the implementation of ROIAlign operation which were found when attempting to run it with the half_pixel coordinate conversion mode, to include more thorough tests. Some bugs are mode-specific and some are not.

The ROIAlign operation was first proposed in a paper at https://arxiv.org/abs/1703.06870v3 which introduced the Mask R-CNN model. It was a variant of the ROIPool operation which was found to give significantly better accuracy. In the implementations in Torch, Onnxruntime, and Migraphx, ROIPool and ROIAlign are implemented in the same op. with different choices for the mode attribute, with output_half_pixel for ROIPool and half_pixel for ROIAlign; thus, there is no ROIAlign op without fixing the half_pixel mode.

Note, by the way, that these same coordinate conversion modes are also attributes of the Resize op.

MIGraphX uses the Onnxruntime implementation of ROIAlign as its functional specification and should give identical results.

This change is prerequisite for torch-migraphx PR #143 but does not close it.

…ng in a dimension of size 1

…tatic.

…X into stride_ordering_for_mlir

…code. Tests need to be completed, including updating generated onnx test files.

…_half_pixel_verify_test for first roi but fails for second

…k in progress with debug code.

…ive correct result only for ROI in bounds.

bpickrel · 2024-10-09T14:57:26Z

Reviewers: This PR isn't quite ready for review as I'm still working on the GPU implementation. I'm just opening it to activate the Jenkins testing.

test/onnx/parse/roialign_default_test.cpp

spolifroni-amd

There's a small wording change but it's neither a big change nor a mandatory one.

… but in mixed up order.

…st_roialign* cases. still contains output index workaround

Co-authored-by: spolifroni-amd <[email protected]>

bpickrel · 2024-10-14T17:20:48Z

This PR is finally ready to begin review. Note that I left a "todo" about recalculating the indexing order in the Reference op. If we come up with a way to iterate through the array in correct order the first time through, great. But getting the order right to this point (just to obtain correct test values) has been very time-consuming and if a fix doesn't present itself, it may be better to stick with the workaround.

test/onnx/verify/roialign_verify_test.cpp

…raphX into roialign_fix

shivadbhavsar

Looks fine, just a few small things. I haven't been able to fully wrap my head around all the math in the ref and gpu impl, the index changes look reasonable. Do we have a way to directly test against ORT (without maunally extracting gold outputs)? If so, I think it would be worthwhile to add a few more tests comparing with ORT

shivadbhavsar · 2024-10-21T18:26:29Z

test/onnx/parse/roialign_default_test.cpp

    EXPECT(p == prog);
+
+    // Opset 12 program


I suggest making this a separate test so if there is a failure you know which one of the two opsets had the problem

tools/build_and_test_onnxrt.sh

test/verify/test_roialign_nondefault.cpp

migraphx-bot · 2024-10-22T20:01:25Z

Test	Batch	Rate new 4c4846	Rate old b73def	Diff	Compare
torchvision-resnet50	64	3,260.10	3,257.93	0.07%	✅
torchvision-resnet50_fp16	64	6,997.72	6,992.99	0.07%	✅
torchvision-densenet121	32	2,431.22	2,432.26	-0.04%	✅
torchvision-densenet121_fp16	32	4,069.01	4,038.39	0.76%	✅
torchvision-inceptionv3	32	1,638.16	1,638.89	-0.04%	✅
torchvision-inceptionv3_fp16	32	2,760.41	2,761.69	-0.05%	✅
cadene-inceptionv4	16	775.87	776.39	-0.07%	✅
cadene-resnext64x4	16	808.62	811.37	-0.34%	✅
slim-mobilenet	64	7,530.01	7,532.73	-0.04%	✅
slim-nasnetalarge	64	211.48	211.42	0.03%	✅
slim-resnet50v2	64	3,502.80	3,507.25	-0.13%	✅
bert-mrpc-onnx	8	1,150.20	1,147.76	0.21%	✅
bert-mrpc-tf	1	463.63	469.91	-1.34%	✅
pytorch-examples-wlang-gru	1	421.33	514.96	-18.18%	🔴
pytorch-examples-wlang-lstm	1	382.12	386.61	-1.16%	✅
torchvision-resnet50_1	1	787.04	772.05	1.94%	✅
cadene-dpn92_1	1	417.30	398.73	4.66%	🔆
cadene-resnext101_1	1	382.44	383.67	-0.32%	✅
onnx-taau-downsample	1	343.17	342.33	0.25%	✅
dlrm-criteoterabyte	1	33.33	33.33	-0.01%	✅
dlrm-criteoterabyte_fp16	1	52.73	52.70	0.06%	✅
agentmodel	1	8,040.01	8,056.20	-0.20%	✅
unet_fp16	2	58.84	58.92	-0.14%	✅
resnet50v1_fp16	1	948.15	950.32	-0.23%	✅
resnet50v1_int8	1	979.13	1,000.02	-2.09%	✅
bert_base_cased_fp16	64	1,171.83	1,169.24	0.22%	✅
bert_large_uncased_fp16	32	363.50	363.69	-0.05%	✅
bert_large_fp16	1	200.64	198.89	0.88%	✅
distilgpt2_fp16	16	2,203.66	2,203.09	0.03%	✅
yolov5s	1	544.88	540.85	0.74%	✅
tinyllama	1	43.44	43.43	0.01%	✅
vicuna-fastchat	1	173.75	170.64	1.82%	✅
whisper-tiny-encoder	1	418.08	418.21	-0.03%	✅
whisper-tiny-decoder	1	434.02	426.10	1.86%	✅

This build is not recommended to merge 🔴

migraphx-bot · 2024-10-22T20:01:27Z

✅ bert-mrpc-onnx: PASSED: MIGraphX meets tolerance

✅ bert-mrpc-tf: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-gru: PASSED: MIGraphX meets tolerance

✅ pytorch-examples-wlang-lstm: PASSED: MIGraphX meets tolerance

✅ torchvision-resnet50_1: PASSED: MIGraphX meets tolerance

✅ cadene-dpn92_1: PASSED: MIGraphX meets tolerance

✅ cadene-resnext101_1: PASSED: MIGraphX meets tolerance

✅ dlrm-criteoterabyte: PASSED: MIGraphX meets tolerance

✅ agentmodel: PASSED: MIGraphX meets tolerance

✅ unet: PASSED: MIGraphX meets tolerance

✅ resnet50v1: PASSED: MIGraphX meets tolerance

✅ bert_base_cased_fp16: PASSED: MIGraphX meets tolerance

🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output

✅ bert_large: PASSED: MIGraphX meets tolerance

✅ yolov5s: PASSED: MIGraphX meets tolerance

✅ tinyllama: PASSED: MIGraphX meets tolerance

✅ vicuna-fastchat: PASSED: MIGraphX meets tolerance

✅ whisper-tiny-encoder: PASSED: MIGraphX meets tolerance

✅ whisper-tiny-decoder: PASSED: MIGraphX meets tolerance

✅ distilgpt2_fp16: PASSED: MIGraphX meets tolerance

bpickrel · 2024-10-22T20:49:51Z

test/verify/test_roialign.cpp

        mm->add_return({r});

        return p;
    }
 };

+template struct test_roialign_half_pixel<migraphx::shape::float_type>;


Is there any point in adding other data types for the half_pixel (i.e. the "real" ROIAlign) test?

bpickrel · 2024-10-22T20:52:36Z

test/onnx/verify/roialign_half_pixel_verify_test.cpp

+
+    pp["rois"]      = migraphx::argument(srois, rois_data.data());
+    pp["batch_ind"] = migraphx::argument(sbi, bi_data.data());
+    pp["y"]         = migraphx::argument(s, data.data());


It's not clear to me whether Y, the output, is even needed for this test. What role does it play?

Likely because roialign_half_pixel_test.onnx expects it as input. I am not sure why its configured like that. Maybe it needs an allocated buffer to write the output to?
I am not very familiar with the verify onnx tests, I would check if this is common in other tests also

bpickrel · 2024-10-22T20:54:05Z

test/onnx/parse/roialign_test.cpp

@@ -41,7 +41,7 @@ TEST_CASE(roialign_test)
                          {{"coordinate_transformation_mode", "output_half_pixel"},
                           {"spatial_scale", 2.0f},
                           {"output_height", 5},
-                           {"output_width", 5},
+                           {"output_width", 3},


This particular change was a big deal, btw, since the old code appeared to work fine until I gave it an output_height and output_width that were not the same.

shivadbhavsar · 2024-10-22T21:23:49Z

test/onnx/verify/roialign_half_pixel_verify_test.cpp

+    migraphx::shape srois{migraphx::shape::float_type, {2, 4}};
+    std::vector<float> rois_data = {1.1, 0.73, 1.7, 1.13, 1.1, 0.73, 2.6, 1.13};
+    migraphx::shape sbi{migraphx::shape::int64_type, {2}}; // batch_index
+    std::vector<int64_t> bi_data = {0, 1};


for good measure, can you add a tests with the following cases:

Repeated batch indices

Missing batch indices (ie. not all batch items are computed on)

Number of ROIs != batch_size

You can probably just create one test case to get all these. Make the input batch_size 3 and the batch_indices something like {1,2,2,1} (and hence the rois shape will be {4,4})
Would be good to have a gpu verify test for this same case too just to be sure gpu impl matches

spolifroni-amd

A whitespace in the table needs to be removed.

spolifroni-amd · 2024-10-23T14:43:11Z

docs/dev/onnx_operators.rst

@@ -697,7 +697,7 @@ Operator Support Matrix
 |                          |           |                 | functions are                |
 |                          |           |                 | not enabled                  |
 +--------------------------+-----------+-----------------+------------------------------+
-| RoiAlign                 | ✅        | FP8, FP16,      |                              |
+| RoiAlign                 | ✅        | FP8, FP16,      |                               |


bpickrel and others added 30 commits July 17, 2024 22:23

adjust stride ordering rules for standard shape: stride can be anythi…

735e7a6

…ng in a dimension of size 1

add a shape test

92bac55

debug code removed

090c767

fix a test

9e8528f

Merge branch 'develop' into stride_ordering_for_mlir

cd32e55

Merge branch 'develop' into stride_ordering_for_mlir

f654d5f

added shape::compatible_lens() method

22cc5ff

format

c5508e6

refactor the function for testing compatible shapes to a non-member s…

21dddd0

…tatic.

changing recursive equal call

31addfc

conditional conpilation for is_compatible_shape()

02633f2

different workaround for compile problem

68467b6

Merge branch 'develop' into stride_ordering_for_mlir

cf58b3c

misc small fixes

be7a72e

Merge branch 'stride_ordering_for_mlir' of github.com:ROCm/AMDMIGraph…

c7a4920

…X into stride_ordering_for_mlir

Merge branch 'develop' into stride_ordering_for_mlir

32bfc24

changes to compatible check, want to see if this passes jenkins

e0f1695

style

95d7a2f

cleanup method names

a3f40dd

format

4fffb17

style

20fb5bc

comment

2371231

add test subcases for new function

0c6bef7

style

ef1d2f6

Merge branch 'develop' into stride_ordering_for_mlir

d64abcb

bug fix work in progress. Contains fixed source code. Contains debug …

94392aa

…code. Tests need to be completed, including updating generated onnx test files.

reordered lens for iteration shape; added some tests. Passes roialign…

a43303c

…_half_pixel_verify_test for first roi but fails for second

bug fixes and added roialign_half_pixel_verify_test which passes. Wor…

69d0d44

…k in progress with debug code.

test cases 2 rois, fails

dbe18b5

created out of bounds test for roialign. Learned that existing code g…

4cb582e

…ive correct result only for ROI in bounds.

bpickrel added 2 commits October 9, 2024 22:28

work in progress

3978d41

work in progress

d6dd2e1

spolifroni-amd reviewed Oct 10, 2024

View reviewed changes

test/onnx/parse/roialign_default_test.cpp Outdated Show resolved Hide resolved

spolifroni-amd approved these changes Oct 10, 2024

View reviewed changes

work in progress; a lot of debug code

d425d56

bpickrel marked this pull request as draft October 10, 2024 21:20

bpickrel and others added 6 commits October 11, 2024 20:01

work in progress, gpu kernel closer to correct. Gives correct results…

682b653

… but in mixed up order.

work in progress

4e122bc

work in progress with GPU output indexes

dbd28a4

fixed GPU kernel and cleaned up debug code. Passes all test_verify te…

7c91757

…st_roialign* cases. still contains output index workaround

removed a debug file

2da60d1

comment

6f94758

Co-authored-by: spolifroni-amd <[email protected]>

bpickrel marked this pull request as ready for review October 14, 2024 17:15

bpickrel commented Oct 15, 2024

View reviewed changes

test/onnx/verify/roialign_verify_test.cpp Outdated Show resolved Hide resolved

bpickrel added 2 commits October 15, 2024 15:54

misc. cleanup; fixed one Jenkins-only fail; added one more shape check

c1000cf

Merge branch 'roialign_fix' of github.com:ROCmSoftwarePlatform/AMDMIG…

85f7243

…raphX into roialign_fix

causten assigned bpickrel Oct 16, 2024

bpickrel mentioned this pull request Oct 18, 2024

Aten roi align ROCm/torch_migraphx#209

Open

shivadbhavsar reviewed Oct 21, 2024

View reviewed changes

bpickrel added 2 commits October 22, 2024 17:14

revert debugging changes

379dcef

clean up debug code

4c4846e

bpickrel commented Oct 22, 2024

View reviewed changes

shivadbhavsar reviewed Oct 22, 2024

View reviewed changes

spolifroni-amd requested changes Oct 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roialign fix and half_pixel mode support #3482

Roialign fix and half_pixel mode support #3482

bpickrel commented Sep 26, 2024 •

edited

Loading

bpickrel commented Oct 9, 2024

spolifroni-amd left a comment

bpickrel commented Oct 14, 2024

shivadbhavsar left a comment

shivadbhavsar Oct 21, 2024

migraphx-bot commented Oct 22, 2024

migraphx-bot commented Oct 22, 2024

bpickrel Oct 22, 2024

bpickrel Oct 22, 2024

shivadbhavsar Oct 22, 2024

bpickrel Oct 22, 2024

shivadbhavsar Oct 22, 2024

spolifroni-amd left a comment

spolifroni-amd Oct 23, 2024

	\| RoiAlign \| ✅ \| FP8, FP16, \| \|
	\| RoiAlign \| ✅ \| FP8, FP16, \| \|

Roialign fix and half_pixel mode support #3482

Are you sure you want to change the base?

Roialign fix and half_pixel mode support #3482

Conversation

bpickrel commented Sep 26, 2024 • edited Loading

bpickrel commented Oct 9, 2024

spolifroni-amd left a comment

Choose a reason for hiding this comment

bpickrel commented Oct 14, 2024

shivadbhavsar left a comment

Choose a reason for hiding this comment

shivadbhavsar Oct 21, 2024

Choose a reason for hiding this comment

migraphx-bot commented Oct 22, 2024

migraphx-bot commented Oct 22, 2024

bpickrel Oct 22, 2024

Choose a reason for hiding this comment

bpickrel Oct 22, 2024

Choose a reason for hiding this comment

shivadbhavsar Oct 22, 2024

Choose a reason for hiding this comment

bpickrel Oct 22, 2024

Choose a reason for hiding this comment

shivadbhavsar Oct 22, 2024

Choose a reason for hiding this comment

spolifroni-amd left a comment

Choose a reason for hiding this comment

spolifroni-amd Oct 23, 2024

Choose a reason for hiding this comment

bpickrel commented Sep 26, 2024 •

edited

Loading