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

Extend thrust::complex unit tests to prepare for upcoming replacement with std::complex #413

Merged
merged 4 commits into from
Sep 18, 2023

Conversation

Blonck
Copy link
Contributor

@Blonck Blonck commented Sep 7, 2023

To ensure a smooth transition from thrust::complex to std::complex,
the unit tests are extended. This approach will prevent unintended
constraints or extensions to thrust::complex functionality when the
the actual switch is done.

Key Enhancements:

  • Introduce type-promoting operator tests.
  • Generate distinct random values when multiple thrust::complex instances are used.
  • Apply clang-format.

This PR is derived from #260 and has been rebased onto the current main branch

Checklist

@Blonck Blonck requested review from a team as code owners September 7, 2023 17:48
@Blonck Blonck requested review from griwes and ericniebler and removed request for a team September 7, 2023 17:48
@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 7, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Blonck Blonck requested review from gevtushenko and removed request for a team September 7, 2023 17:48
@Blonck
Copy link
Contributor Author

Blonck commented Sep 7, 2023

Hi @miscco, as discussed here is the PR based on #260.

@miscco
Copy link
Collaborator

miscco commented Sep 8, 2023

/ok to test

@miscco
Copy link
Collaborator

miscco commented Sep 11, 2023

/ok to test

thrust/testing/complex.cu Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/unittest/assertions.h Outdated Show resolved Hide resolved
thrust/testing/unittest/assertions.h Outdated Show resolved Hide resolved
thrust/thrust/detail/complex.inl Outdated Show resolved Hide resolved
@Blonck Blonck force-pushed the replace_complex branch 3 times, most recently from 6e092e7 to fb378b0 Compare September 11, 2023 17:17
To ensure a smooth transition from `thrust::complex` to `std::complex`,
the unit tests are extended. This approach will prevent unintended
constraints or extensions to `thrust::complex` functionality when the
the actual switch is done.

Key Enhancements:
- Introduce type-promoting operator tests.
- Generate distinct random values when multiple `thrust::complex` instances are used.
- Apply clang-format.
@miscco
Copy link
Collaborator

miscco commented Sep 12, 2023

/ok to add

@miscco
Copy link
Collaborator

miscco commented Sep 12, 2023

/ok to test

Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

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

It seems that this PR only changes thrust::complex tests. If this is intended, please, change PR name and description. Otherwise if the PR is still work in progress, please, convert it to draft.

@miscco
Copy link
Collaborator

miscco commented Sep 12, 2023

It seems that this PR only changes thrust::complex tests. If this is intended, please, change PR name and description. Otherwise if the PR is still work in progress, please, convert it to draft.

That was me, I asked to first add the tests so that we know that we do not extend the existing API with tests and then put the changing commit on top

@Blonck Blonck changed the title Replace thrust::complex with a struct derived from std::complex Extend thrust::complex unit tests to prepare for upcoming replacement with std::complex Sep 13, 2023
@Blonck
Copy link
Contributor Author

Blonck commented Sep 13, 2023

Changed title and description.

Copy link
Collaborator

@miscco miscco left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks great 🎉

I some minor nits and request for a few additional tests.

@senior-zero I see that we only test on host. We need to expand test coverage to device when we rework thrust test suite to catch2

thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
thrust/testing/complex.cu Show resolved Hide resolved
thrust/testing/complex.cu Outdated Show resolved Hide resolved
@miscco
Copy link
Collaborator

miscco commented Sep 14, 2023

/ok to test

@miscco
Copy link
Collaborator

miscco commented Sep 14, 2023

/ok to test

@miscco miscco merged commit f591fa4 into NVIDIA:main Sep 18, 2023
463 checks passed
@Blonck Blonck deleted the replace_complex branch September 18, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants