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.

Sign up for GitHub

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

Added fix for test_msgpack_float and test_msgpack_object #2155

Merged
merged 11 commits into from
Oct 6, 2023
Merged

Added fix for test_msgpack_float and test_msgpack_object #2155

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f557e67
Added fix for test_msgpack_float and test_msgpack_object
tvukovic-amd Sep 4, 2023
3c400ba
Added warning fix for double equal
tvukovic-amd Sep 27, 2023
ec541ae
Merge branch 'develop' into fix/msgpack_test
apwojcik Sep 28, 2023
bf42d61
Added test::within_abs in comparing float values
tvukovic-amd Sep 29, 2023
959a01c
Added test::within_abs in comparing float values
tvukovic-amd Sep 29, 2023
22a9ea0
Merge branch 'develop' into fix/msgpack_test
causten Oct 1, 2023
f391728
Merge branch 'develop' into fix/msgpack_test
causten Oct 3, 2023
9b5a580
Added Windows fix for double inputs in test
tvukovic-amd Oct 5, 2023
cea31cc
Added Windows fix for double inputs in test
tvukovic-amd Oct 5, 2023
d2fd5ac
Added comments why double inputs are changed
tvukovic-amd Oct 5, 2023
8d1157c
Merge branch 'develop' into fix/msgpack_test
causten Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions test/msgpack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ TEST_CASE(test_msgpack_float)
migraphx::value v = 3.0;
auto buffer = migraphx::to_msgpack(v);
EXPECT(buffer == msgpack_buffer(3.0));
EXPECT(migraphx::from_msgpack(buffer) == v);
EXPECT(migraphx::from_msgpack(buffer).to<float>() == v.to<float>());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to be converted to float first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Function migraphx::from_msgpack returns value which needs to be converted to other types (except for string and bool case) when we are comparing output values. The tests above convert value to appropriate type (here is the example for int).

}

TEST_CASE(test_msgpack_string)
Expand Down Expand Up @@ -109,7 +109,11 @@ TEST_CASE(test_msgpack_object)
auto buffer = migraphx::to_msgpack(v);
EXPECT(buffer == msgpack_buffer(std::map<std::string, double>{
{"one", 1.0}, {"three", 3.0}, {"two", 2.0}}));
EXPECT(migraphx::from_msgpack(buffer) == v);

// converted to vector in the following line because in value.cpp value constructor with
// unordered map is creating vector<value> with map items as vector elements
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where are we using an unordered map?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created buffer is a map. After its creation there is check if buffer is equal with buffer initialized with map (here).
In value definition inside value.hpp I haven't seen constructor for map, I found just constructor for unordered map, so because of that I used conversion to vector.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In value definition inside value.hpp I haven't seen constructor for map, I found just constructor for unordered map

Its not using the std::unordered_map constructor, its using the std::initializer_list constructor which will preserve the order of the list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw value constructor for std::initializer_list in value.cpp. In this case (where size is not 2 and elements have key) it is calling set_vector function, so I think to_vector() function can be used for comparing if result values are equal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to_vector will only compare the values and not the keys. The == operator for value will compare the keys as well: migraphx::from_msgpack(buffer) == v.

Copy link
Collaborator Author

@tvukovic-amd tvukovic-amd Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If migraphx::from_msgpack(buffer) function is fixed, I won't have need to cast output to float in the following way which i committed in this PR too (EXPECT(migraphx::from_msgpack(buffer).to<float>() == v.to<float>());).

Copy link
Collaborator

Choose a reason for hiding this comment

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

but the problem here is that migraphx::from_msgpack(buffer) converts any numeric value to uint64_t

Where do you see this happening? from_msgpack converts the float32 or float64 to double here: https://github.com/ROCmSoftwarePlatform/AMDMIGraphX/blob/develop/src/msgpack.cpp#L85.

Copy link
Collaborator Author

@tvukovic-amd tvukovic-amd Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see it in the debugger during comparing EXPECT(migraphx::from_msgpack(buffer) == v);. You can see debugger output in the following screenshot when debugging test_msgpack_float, here x is migraphx::from_msgpack(buffer) output and y is migraphx::value v which is double. If we cast v to be uint64_t, the test passes.
Also, if there is type conversion why do we need to cast function output to int here (which already exists in develop branch)?
image

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion since we do want to keep comparing the keys and the types are different for the values that we should either change operator== to have a specialization that can properly compare types, call the compare function directly if that will allow us to compare types or create a compare function that can compare the keys and compare with an explicit type conversion (passed as a template parameter).

Copy link
Collaborator

Choose a reason for hiding this comment

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

So there is something wrong with either msgpack serialization or the value construction as it should be float_type as well.

// value(std::vector<value>(m.begin(), m.end()), false)
EXPECT(migraphx::from_msgpack(buffer).to_vector<double>() == v.to_vector<double>());
}

TEST_CASE(test_msgpack_empty_object)
Expand Down