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

Replace assert with runtime error #122

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

yasahi-hpc
Copy link
Collaborator

@yasahi-hpc yasahi-hpc commented Jul 24, 2024

Improves #80

This PR aims at replacing runtime assert with std::runtime_error with appropriate messages.
If there is inconsistency in extents, we cannot operate FFT.

Modifications are applied to get_shift, get_extents, get_modified_shape and convert_negative_axis. For example, in get_shift, we replaced assert with std::runtime_error in the following way.

// Previous check
assert(!KokkosFFT::Impl::has_duplicate_values(axes));

// Current check
//if (KokkosFFT::Impl::has_duplicate_values(axes)) {
//    throw std::runtime_error("get_shift: axes are overlapped.");
//}
//check_precondition(!KokkosFFT::Impl::has_duplicate_values(axes),
//                   "axes are overlapped");
KOKKOSFFT_EXPECTS(!KokkosFFT::Impl::has_duplicate_values(axes),
                  "axes are overlapped");

Following modifications are made

  • Replacing runtime assert with std::runtime_error
  • Add corresponding tests to check std::runtime_error is correctly thrown.
  • Add missing test for has_duplicate_values
  • Introduce check_precondition(expression, message) function KOKKOSFFT_EXPECTS(expression, message)macro

Sorry, something went wrong.

@yasahi-hpc yasahi-hpc self-assigned this Jul 24, 2024
@dalg24
Copy link
Member

dalg24 commented Jul 24, 2024

Maybe you could abstract that away and have a precondition check utility that takes a message as argument.

@yasahi-hpc
Copy link
Collaborator Author

Maybe you could abstract that away and have a precondition check utility that takes a message as argument.

Makes sense. So I would introduce something like this?
I will do that in another PR

void throw_runtime_error(const bool condition, const std::string &msg) {
  if (condition) {
    throw std::runtime_error(msg);
  }
}

@dalg24
Copy link
Member

dalg24 commented Jul 24, 2024

You probably want to call it something else, possibly check_precondition(expression, message).
If your minimum requirement allows it, consider having a std::source_location trailing argument defaulted to "current". Otherwise you could look into using a macro.

@yasahi-hpc
Copy link
Collaborator Author

You probably want to call it something else, possibly check_precondition(expression, message). If your minimum requirement allows it, consider having a std::source_location trailing argument defaulted to "current". Otherwise you could look into using a macro.

That is a better name. We are currently supporting C++17, so maybe using a macro.
I am also interested in introducing C++20 features, so I will give it a try with a protection by KOKKOS_ENABLE_CXX17 macro

@cedricchevalier19
Copy link
Member

@yasahi-hpc: do you have strong requirements from users for supporting C++17 and not requiring C++20?
I think if we can avoid #ifdef it will be a lot better.

@yasahi-hpc
Copy link
Collaborator Author

@yasahi-hpc: do you have strong requirements from users for supporting C++17 and not requiring C++20? I think if we can avoid #ifdef it will be a lot better.

I do not think so. I was just planning to require C++20 at the same time as Kokkos.

@yasahi-hpc
Copy link
Collaborator Author

@cedricchevalier19
For the moment, I am relying on macros. Is this OK for you?

common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

@dalg24 I guess is OK now. Can I have your second review please

@yasahi-hpc yasahi-hpc requested a review from dalg24 July 25, 2024 13:17
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I haven't looked in details but it looks like the right direction

common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

I haven't looked in details but it looks like the right direction

I think so. I am inclined to improve the compile time and runtime errors. Still insufficient though

@yasahi-hpc yasahi-hpc requested a review from dalg24 July 25, 2024 15:58
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

BTW, are you sure the CI tests both C++20 and no C++20?

common/src/KokkosFFT_Helpers.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_Helpers.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_layouts.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_layouts.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_padding.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_padding.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
common/src/KokkosFFT_utils.hpp Outdated Show resolved Hide resolved
@yasahi-hpc
Copy link
Collaborator Author

BTW, are you sure the CI tests both C++20 and no C++20?

I was planning to add C++20 after this PR, but it may be better to do that before

@yasahi-hpc yasahi-hpc force-pushed the replace-assert-with-runtime-error branch from 30ea12f to 2a1d1d9 Compare July 29, 2024 13:02
@yasahi-hpc yasahi-hpc force-pushed the replace-assert-with-runtime-error branch from 75b7015 to 07282b9 Compare July 29, 2024 17:06
Copy link
Member

@cedricchevalier19 cedricchevalier19 left a comment

Choose a reason for hiding this comment

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

LGTM

@yasahi-hpc yasahi-hpc merged commit 0b1ab3d into kokkos:main Jul 30, 2024
19 checks passed
@yasahi-hpc yasahi-hpc deleted the replace-assert-with-runtime-error branch July 30, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants