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

Introduce fast and deterministic RNG #615

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

Cuda-Chen
Copy link
Collaborator

@Cuda-Chen Cuda-Chen commented Oct 6, 2023

Use SplitMix64 for fast and deterministic RNG for cross compiler/platform support.

@jserv jserv requested a review from howjmay October 6, 2023 06:16
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 6578a55 to 8a021b6 Compare October 6, 2023 06:16
tests/impl.cpp Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 8a021b6 to 08a9a1c Compare October 6, 2023 06:19
tests/impl.cpp Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 08a9a1c to 3f8d5eb Compare October 6, 2023 06:24
@Cuda-Chen Cuda-Chen marked this pull request as ready for review October 6, 2023 07:39
@Cuda-Chen Cuda-Chen requested a review from marktwtn as a code owner October 6, 2023 07:39
@Cuda-Chen Cuda-Chen requested a review from jserv October 6, 2023 07:39
tests/impl.cpp Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 3f8d5eb to 4bf7fbe Compare October 6, 2023 07:51
tests/impl.cpp Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 4bf7fbe to b535ac1 Compare October 6, 2023 08:25
tests/impl.cpp Outdated Show resolved Hide resolved
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from b535ac1 to 9af9c80 Compare October 6, 2023 13:57
@Cuda-Chen Cuda-Chen requested a review from jserv October 6, 2023 14:07
@howjmay
Copy link
Collaborator

howjmay commented Oct 6, 2023

I am curious. Doesn't it still generate the same random number sequence?
I concerned about we may not cover all the possible values with the previous RNG function.
But maybe having a consistent random number sequence is better?

@Cuda-Chen
Copy link
Collaborator Author

I am curious. Doesn't it still generate the same random number sequence? I concerned about we may not cover all the possible values with the previous RNG function. But maybe having a consistent random number sequence is better?

After searching, I realize that "consistent random number sequence" means the RNG always produces the same random number sequence every time the test program is run. Is that what we are going to discuss about?

@howjmay
Copy link
Collaborator

howjmay commented Oct 7, 2023

I am curious. Doesn't it still generate the same random number sequence? I concerned about we may not cover all the possible values with the previous RNG function. But maybe having a consistent random number sequence is better?

After searching, I realize that "consistent random number sequence" means the RNG always produces the same random number sequence every time the test program is run. Is that what we are going to discuss about?

Yes that is exactly what I mentioned. So do we need to have a consistent random number sequence or maybe not?

@jserv
Copy link
Member

jserv commented Oct 7, 2023

After searching, I realize that "consistent random number sequence" means the RNG always produces the same random number sequence every time the test program is run. Is that what we are going to discuss about?

Yes that is exactly what I mentioned. So do we need to have a consistent random number sequence or maybe not?

For the purpose of CI pipeline, we shall stick to deterministic random number generation.

@howjmay
Copy link
Collaborator

howjmay commented Oct 7, 2023

After searching, I realize that "consistent random number sequence" means the RNG always produces the same random number sequence every time the test program is run. Is that what we are going to discuss about?

Yes that is exactly what I mentioned. So do we need to have a consistent random number sequence or maybe not?

For the purpose of CI pipeline, we shall stick to deterministic random number generation.

Got it.
Then I am curious the purpose of this PR. The old implementation has already presented a deterministic random number generation. Maybe we should just close the original issue?

@Cuda-Chen
Copy link
Collaborator Author

After searching, I realize that "consistent random number sequence" means the RNG always produces the same random number sequence every time the test program is run. Is that what we are going to discuss about?

Yes that is exactly what I mentioned. So do we need to have a consistent random number sequence or maybe not?

For the purpose of CI pipeline, we shall stick to deterministic random number generation.

Got it. Then I am curious the purpose of this PR. The old implementation has already presented a deterministic random number generation. Maybe we should just close the original issue?

I agree. Just close the original issue with a comment mentioning about sticking to deterministic random number generation of CI pipelines.

Copy link
Member

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Refine the subject per discussions.

@howjmay
Copy link
Collaborator

howjmay commented Oct 7, 2023

issue was closed

@Cuda-Chen
Copy link
Collaborator Author

As the rand() usually acts good for deterministic random number generation, I would like to close this PR if we do not need fast random number generation.

@jserv jserv changed the title Use SplitMix64 for seeding test cases Introduce fast and deterministic RNG Oct 8, 2023
@jserv
Copy link
Member

jserv commented Oct 8, 2023

As the rand() usually acts good for deterministic random number generation, I would like to close this PR if we do not need fast random number generation.

The proposed RNG still makes sense for cross-platform consideration. In particular, this project supports GCC, Clang, and MSVC in combination with Linux, macOS, and Windows. Therefore, a fast and deterministic RNG can minimize potential issues while performing tests.

@Cuda-Chen, please refine the commit messages.

Use SplitMix64 for fast and deterministic RNG for cross
compiler/platform support.
@Cuda-Chen Cuda-Chen force-pushed the gemerate-more-random-test-inputs branch from 9af9c80 to 4142eee Compare October 8, 2023 02:46
@Cuda-Chen Cuda-Chen requested a review from jserv October 8, 2023 02:51
@jserv jserv merged commit 3497b37 into DLTcollab:master Oct 8, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants