Implementing parsing of Mean Variance Normalization by gchinora · Pull Request #202 · migraphx-benchmark/AMDMIGraphX

gchinora · 2025-03-31T08:08:44Z

No description provided.

…r test, verify test and associated onnx files

dinomusic · 2025-03-31T08:54:14Z

+        }
+        assert(X->get_shape().ndim() >= axes_min_size);
+
+        auto E_X        = info.add_instruction(make_op("reduce_mean", {{"axes", axes}}), X);


Using snake case is correct, but you shouldn't use capital letters in local variable names

Fair enough.... I was trying to confirm to the mathematical symbol of the expected value of a random variable. Which is E(X).

dinomusic · 2025-03-31T08:56:34Z

+            axes.assign(info.attributes.at("axes").ints().begin(), info.attributes.at("axes").ints().end());
+            axes_min_size = axes.size();
+        }
+        assert(X->get_shape().ndim() >= axes_min_size);


In the parser you should throw an exception when a condition isn't met. You can do this using the MIGRAPHX_THROW macro which takes a string as input. There isn't a set format for the string, I usually go with "OperatorName: input_dim/attribute/anything has value x, should be y", something like you have above for the type validty check.

Fair enough... note: see parse_instancenorm.cpp:88 there's also an assert(.) call.

dinomusic · 2025-03-31T09:08:41Z

+
+        const auto& X                  = args[0];
+
+        const auto eps_default            = 1e-9f;


This isn't something that's pointed out in the docs, but 1e-9f wouldn't be the correct value to use for fp16, it's too small.

dinomusic · 2025-03-31T09:09:11Z

+struct mean_variance_norm : op_parser<mean_variance_norm>
+{
+    std::set<shape::type_t> valid_types = {
+        shape::bf16_type, shape::double_type, shape::float_type};


Missing fp16 from supported type list.

dinomusic · 2025-03-31T09:16:33Z

+@onnx_test()
+def mean_variance_norm_val_test():
+    # example from: https://github.com/onnx/onnx/blob/main/onnx/backend/test/case/node/meanvariancenormalization.py
+    x = np.array(


Usually we want to avoid specifying tensor values in the graph, the instancenorm tests are an outlier in this regard.
By doing this you can reuse the same graph for the parse and verify tests.
You'd provide the values as an input to the compiled migraphx program.

Have a look at how gen_onnx.py:mod_test() does it, and its corresponding cpp file test/onnx/verify/mod_test.cpp

dinomusic · 2025-03-31T09:18:59Z

+    node = onnx.helper.make_node('MeanVarianceNormalization',
+                                    inputs=['x'],
+                                    outputs=['y'],
+                                    axes=ax,


You should also add a test where you don't define the axes attribute to check if the parser correctly defaults it.

dinomusic · 2025-03-31T09:20:06Z

Negative parser tests should also be added. If there's a branch in your parser code that leads to an MIGRAPHX_THROW, it should be covered with a test.

dinomusic · 2025-03-31T09:23:30Z

+    mm->add_return({Y});
+
+    migraphx::onnx_options options;
+    auto prog = read_onnx("mean_variance_norm_test.onnx", options);


Should use optimize_onnx to parse the model, read_onnx was used before so older tests still contain it.

dinomusic · 2025-03-31T09:24:21Z

+    auto denominator= add_common_op(*mm, migraphx::make_op("add"), {std, eps_literal});
+    auto Y          = add_common_op(*mm, migraphx::make_op("div"), {numerator, denominator});
+
+    mm->add_return({Y});


Probably won't need the add_return when using optimize_onxx().

…onnx file; modify tensor shapes

… in the gen_onnx.py file

dinomusic · 2025-04-07T10:32:19Z

+
+        const auto& x           = args[0];
+
+        const auto eps_default  = 1e-7f;


You'd want eps_default to be 1e-7 if dtype is half_type, and 1e-9 for the other types.

dinomusic · 2025-04-07T10:37:35Z

+    migraphx::program p;
+    const auto prog = optimize_onnx("mean_variance_norm_default_axes_test.onnx");
+    const auto* mm = prog.get_main_module();
+    const auto it = std::find_if(mm->begin(), mm->end(), [](auto instr){ return instr.name() == "reduce_mean";});


I like the approach here. You'd still want to build out an entire "expected" graph for these tests as that's the approach in migraphx, but kudos for the way you did it. You don't need to make a change to this.

HTEC added 3 commits March 26, 2025 13:38

implementing the parsing of mean variance normalization; adding parse…

30eda28

…r test, verify test and associated onnx files

have the type check at the top of the parsing function

f395a8f

tiny renaming

650adc4

dinomusic reviewed Mar 31, 2025

View reviewed changes

gybacsi and others added 16 commits April 1, 2025 11:09

'resolving review comments part 1'

b5e470a

add missing semicolon

0a371b2

change epsilon default in parser test too

ce9da7e

ident fix

f86da7f

renaming; using optimize_onnx instead of read_onny

01372ec

remove onnx generator that included tensor values; remove associated …

817769f

…onnx file; modify tensor shapes

add new onnx file; modify test a bit according to how it is generated…

67cdc0c

… in the gen_onnx.py file

delete onnx with wrong axes value

e10e776

yet another onnx file and the modified test

f660e4a

reuse of existing helper function

38e132a

'reuse existing helper function'

8148b1d

fix; change parameter name in test

f8f4883

'Add test; testing if axes are defaulted if not specified'

3dfd9ae

'removing data from python and moving it to the verify test'

ab9928f

'adding test for invalid type case'

85a4ec9

'adding test for invalid input axes shape'

9725f4b

gchinora requested a review from dinomusic April 2, 2025 09:50

dinomusic reviewed Apr 7, 2025

View reviewed changes

'set epsilon according to input type'

37654c3

gchinora requested a review from dinomusic April 8, 2025 09:47

Conversation

gchinora commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dinomusic Mar 31, 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!

dinomusic Mar 31, 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!

dinomusic commented Mar 31, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dinomusic Mar 31, 2025 •

edited

Loading

dinomusic Mar 31, 2025 •

edited

Loading