[RNG][Device API] Added (u)int8_t and (u)int16_t types support for Uniform #632
Conversation
| if constexpr (std::is_same_v<Type, std::int32_t> || | ||
| std::is_same_v<Type, std::uint32_t>) { | ||
| if constexpr (std::is_same_v<Type, std::int8_t> || std::is_same_v<Type, std::uint8_t> || | ||
| std::is_same_v<Type, std::int16_t> || std::is_same_v<Type, std::uint16_t>) { |
There was a problem hiding this comment.
I would suggest to move this dispatch above to have floating type definition in one place, e.g.
std::conditional_t<
!std::is_same_v<Method, uniform_method::accurate> ||
std::is_same_v<Type, std::int8_t> ||
std::is_same_v<Type, std::uint8_t> ||
std::is_same_v<Type, std::int16_t> ||
std::is_same_v<Type, std::uint16_t>,
float, double>There was a problem hiding this comment.
I think it looks clearer with the extra condition. Furthermore, the existing condition relates to (u)int32, (u)int64 and floating point precision types. But with the option you suggested we will have less additional code.
@iMartyan, could you please share your opinion here?
There was a problem hiding this comment.
I also like Andrey's approach more because the generation method is the same. So, better not to split it into several branches.
| ParamsType a = distr.a(); | ||
| ParamsType b = distr.b(); |
There was a problem hiding this comment.
Is it OK if a, b params are always double-precision floating point numbers for all small types?
There was a problem hiding this comment.
Could you please check it? If tests are passed I don't see any objections to use double for all Fp values
There was a problem hiding this comment.
I launched moments manually and they successfully passed
There was a problem hiding this comment.
So, I'm fine if we switch to double if there are no objections from somebody else
Description
At the DPNP team request support for the following small types was added for Uniform distribution:
Checklist
All Submissions