Fix mean computation for the geometric distribution in the data generator #15282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vuule
added
bug
|
CC @shrshi |
|
Parquet reader benchmarks with only list columns are faster by 50%, while the ones with mixed types are 20-30X faster. |
vuule
commented
Mar 13, 2024
| // In the current implementation, the geometric distribution is | ||
| // approximated by absolute value of a uniform distribution | ||
| auto const gauss_std_dev = geometric_as_gauss_std_dev(dist.lower_bound, dist.upper_bound); | ||
| auto const half_gauss_mean = gauss_std_dev * sqrt(2. / M_PI); |
There was a problem hiding this comment.
PointKernel
approved these changes
Mar 13, 2024
…bug-geometric-distribution-mean
|
@PointKernel made some additional changes; requested your review so I don't sneak them past you :) |
PointKernel
approved these changes
Mar 14, 2024
pmattione-nvidia
approved these changes
Mar 14, 2024
shrshi
approved these changes
Mar 14, 2024
…bug-geometric-distribution-mean
…vuule/cudf into bug-geometric-distribution-mean
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Since we moved random data generation to the GPU, geometric distribution has been approximated by half-normal distribution. However, the mean computation wasn't updated, causing a ~20% higher mean that the actual generated values.
Another issue that exasperated the problem is the implicit conversion to ints in the random generator. This effectively lowered the mean of generated values by 0.5.
Together, these lead to list columns having the last row with more than 20% of the total column data. Huge single row caused low performance in many benchmarks. For example, Parquet files end up with a few huge pages and load imbalance in decode.
This PR fixes the mean computation to reflex the actual distribution, and rounds the random values when converting to ints. The result is a correct distribution of the number of elements in each randomly generated list.
Checklist