[libcu++] Support complex type mixing in cuda::isclose#9891
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe complex ChangesComplex isclose overloads
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5d13923-bb53-4ce0-948b-2d73e381da6f
📒 Files selected for processing (3)
docs/libcudacxx/extended_api/numeric/isclose.rstlibcudacxx/include/cuda/__numeric/isclose.hlibcudacxx/test/libcudacxx/cuda/numeric/isclose/isclose.pass.cpp
| template <class T> | ||
| [[nodiscard]] __host__ __device__ | ||
| bool isclose(const Complex& lhs, const Complex& rhs) noexcept; | ||
| bool isclose(const /*complex-type*/<T>& lhs, const /*complex-type*/<T>& rhs) noexcept; | ||
|
|
||
| template <class Complex> | ||
| template <class T> | ||
| [[nodiscard]] __host__ __device__ | ||
| bool isclose(const Complex& lhs, const Complex& rhs, float relative_tol) noexcept; | ||
| bool isclose(const /*complex-type*/<T>& lhs, const /*complex-type*/<T>& rhs, float relative_tol) noexcept; | ||
|
|
||
| template <class Complex, class AbsTol> | ||
| template <class T> | ||
| [[nodiscard]] __host__ __device__ | ||
| bool isclose(const Complex& lhs, | ||
| const Complex& rhs, | ||
| bool isclose(const /*complex-type*/<T>& lhs, | ||
| const /*complex-type*/<T>& rhs, | ||
| float relative_tol, | ||
| AbsTol absolute_tol) noexcept; | ||
| T absolute_tol) noexcept; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
important: Document cuda::complex<T> and independently selectable lhs/rhs complex types. The signatures repeat one /*complex-type*/ placeholder and Line 70 lists only cuda::std::complex<T> and std::complex<T>, but the new tested API supports cuda::std::complex<T> mixed with cuda::complex<T>. State that both operands may use different supported complex implementations with the same scalar T. As per path instructions, documentation changes must be technically accurate and API-consistent.
Also applies to: 70-70
Source: Path instructions
| _CCCL_TEMPLATE(class _Tp, template <class> class _LhsComplex, template <class> class _RhsComplex) | ||
| _CCCL_REQUIRES(::cuda::std::__cccl_is_integer_v<_Tp> || ::cuda::is_floating_point_v<_Tp>) | ||
| [[nodiscard]] _CCCL_HOST_DEVICE_API bool isclose( | ||
| const _LhsComplex<_Tp>& __lhs, const _RhsComplex<_Tp>& __rhs, const float __rel_tol, const _Tp __abs_tol) noexcept |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Restrict all complex overloads to floating-point _Tp. Lines 290, 304, and 317 admit integral _Tp, but Line 96 hard-errors after overload selection. Use ::cuda::is_floating_point_v<_Tp> consistently so unsupported complex scalar types are excluded by constraints rather than failing in the function body; add constraint coverage for this case. As per path instructions, libcudacxx changes require correctness and API stability.
Also applies to: 303-320
Source: Path instructions
This comment has been minimized.
This comment has been minimized.
| template <class _Tp, template <class> class _LhsComplex, template <class> class _RhsComplex> | ||
| [[nodiscard]] _CCCL_HOST_DEVICE_API bool __isclose_complex_impl( | ||
| const _ComplexType& __lhs, const _ComplexType& __rhs, const float __rel_tol, const _AbsTol __abs_tol) noexcept | ||
| const _LhsComplex<_Tp>& __lhs, const _RhsComplex<_Tp>& __rhs, const float __rel_tol, const _Tp __abs_tol) noexcept |
There was a problem hiding this comment.
If complex type mixing is not desired, we can do:
| template <class _Tp, template <class> class _LhsComplex, template <class> class _RhsComplex> | |
| [[nodiscard]] _CCCL_HOST_DEVICE_API bool __isclose_complex_impl( | |
| const _ComplexType& __lhs, const _ComplexType& __rhs, const float __rel_tol, const _AbsTol __abs_tol) noexcept | |
| const _LhsComplex<_Tp>& __lhs, const _RhsComplex<_Tp>& __rhs, const float __rel_tol, const _Tp __abs_tol) noexcept | |
| template <class _Tp, template <class> class _Complex> | |
| [[nodiscard]] _CCCL_HOST_DEVICE_API bool __isclose_complex_impl( | |
| const _Complex<_Tp>& __lhs, const _Complex<_Tp>& __rhs, const float __rel_tol, const _Tp __abs_tol) noexcept |
| template <class T> | ||
| [[nodiscard]] __host__ __device__ | ||
| bool isclose(const Complex& lhs, const Complex& rhs) noexcept; | ||
| bool isclose(const /*complex-type*/<T>& lhs, const /*complex-type*/<T>& rhs) noexcept; |
There was a problem hiding this comment.
Is this functionally any different from the previous form? From your change it seems like what we actually support is
template <class LhsComplex, class RhsComplex>
isclose(const LhsComplex&, const RhsComplex&);| } | ||
|
|
||
| template <typename _ComplexType, typename _AbsTol> | ||
| template <class _Tp, template <class> class _LhsComplex, template <class> class _RhsComplex> |
There was a problem hiding this comment.
If we are going to be truly generic to the complex type implementation here, then we should not enforce only a single template parameter and should just take a fully generic ComplexType. We can extract the _Tp inside the function using typename ComplexType::value_type. If we are concerned this is too broad, then we can make a is_complex_type concept that we static_assert() inside the function:
template <class T>
concept is_complex_type = requires(const T& cplx) {
{ cplx.imag() } -> ...
{ cplx.real() } -> ...
// etc
};
fbusato
left a comment
fbusato
left a comment
There was a problem hiding this comment.
I'm not really in favor of this change. It would be weird if a user mixes two different types of complex types (cuda::complex vs. cuda::std::complex) in the same call.
The additional complexity does not motivate this change
🥳 CI Workflow Results🟩 Finished in 1h 36m: Pass: 100%/120 | Total: 1d 07h | Max: 57m 48s | Hits: 98%/366831See results here. |
3 participants
#9577 got merged before my feedback was discussed/addressed. This PR simplifies the constraints on complex types and adds support for complex type mixing (for example
cuda::isclose(cuda::std::complex<float>{}, cuda::complex<float>{})incuda::isclose