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

Fix error aspect fp64 is not supported in tests of std::complex #2029

Merged
merged 5 commits into from
Feb 3, 2025

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Jan 30, 2025

In this PR we fix the error Required aspect fp64 is not supported on the device in some tests of std::complex.

@SergeyKopienko SergeyKopienko added the test Test only Change label Jan 30, 2025
@SergeyKopienko SergeyKopienko added this to the 2022.8.0 milestone Jan 30, 2025
@SergeyKopienko SergeyKopienko changed the title Fix error Required aspect fp64 is not supported on the device in tests of std::complex Fix error aspect fp64 is not supported in tests of std::complex Jan 30, 2025
@SergeyKopienko SergeyKopienko added test Test only Change and removed test Test only Change labels Jan 30, 2025
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev left a comment

Choose a reason for hiding this comment

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

LGTM

@danhoeflinger
Copy link
Contributor

As discussed offline, I'm not sure how this should help this error. The numeric literals should all be integers, which shouldn't cause problems being confused for doubles. We already seem to protect against testing cases with doubles with our macros, so its only floats that we cast to. These should already be implicitly converting due to constexpr constructor of std::complex, so I'd expect no change to the issue.

I see these changes as harmless, but I don't understand how they help either.

@danhoeflinger
Copy link
Contributor

danhoeflinger commented Jan 30, 2025

So, I think your new changes will avoid the problem.
Here is the comment from the definition of TestUtils::infinity_val:

    // This constant was declared to avoid the substitution and evaluation of the macro INFINITY
    // inside Kernel code. Since the implementation of INFINITY macro is not regulated, there are
    // cases where temporary expressions of type double are used to calculate the value of INFINITY.
    // This lead to an error on devices where the double type is not supported.

However, it effectively just removes any testing of this for devices not supporting double. Can we pick a different baseline check for these with floating point which doesn't use TestUtils::infinity_val`?

@akukanov
Copy link
Contributor

akukanov commented Jan 30, 2025

This is clearly not a proper fix. If the test is supposed to check the use of floats on a device that does not support doubles, obviously the test itself should not use doubles either.

If INFINITY does not work for some reason, there are alternatives worth checking: https://en.cppreference.com/w/cpp/types/numeric_limits/infinity, https://en.cppreference.com/w/cpp/numeric/math/HUGE_VAL

@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/fix_std_complex_tests_fp64 branch from 6b9a56e to 6226b4a Compare January 31, 2025 11:07
@SergeyKopienko
Copy link
Contributor Author

So, I think your new changes will avoid the problem. Here is the comment from the definition of TestUtils::infinity_val:

    // This constant was declared to avoid the substitution and evaluation of the macro INFINITY
    // inside Kernel code. Since the implementation of INFINITY macro is not regulated, there are
    // cases where temporary expressions of type double are used to calculate the value of INFINITY.
    // This lead to an error on devices where the double type is not supported.

However, it effectively just removes any testing of this for devices not supporting double. Can we pick a different baseline check for these with floating point which doesn't use TestUtils::infinity_val`?

All other significant changes are packed into test_edges() which also required double type support.

Copy link
Contributor

@MikeDvorskiy MikeDvorskiy left a comment

Choose a reason for hiding this comment

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

LGTM

@SergeyKopienko SergeyKopienko merged commit 3382895 into main Feb 3, 2025
22 checks passed
@SergeyKopienko SergeyKopienko deleted the dev/skopienko/fix_std_complex_tests_fp64 branch February 3, 2025 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Test only Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants