Skip to content

Conversation

@mzient
Copy link
Contributor

@mzient mzient commented Nov 25, 2025

Category:

New feature (non-breaking change which adds functionality)
Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

This PR adds Poisson distribution implemented with cuRAND for GPU and std::poisson_distribution for CPU.
The tests cover distribution histograms.
There's also significant refactoring of the random distribution tests.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

@greptile-apps
Copy link

greptile-apps bot commented Nov 25, 2025

Greptile Overview

Greptile Summary

This PR adds Poisson distribution support using curand_poisson for GPU and std::poisson_distribution for CPU, along with significant test refactoring. The implementation includes:

  • Enhanced Philox4x32_10 with UniformRandomBitGenerator interface (result_type, min(), max()) for C++ standard library compatibility
  • New poisson_dist struct with separate overloads for GPU (CurandGenerator) and CPU (generic RNG)
  • CurandGenerator wrapper moved from test code to main header for broader reusability
  • Comprehensive test coverage comparing CPU and GPU histogram distributions
  • Test infrastructure refactored into shared header file (random_dist_test.h) eliminating code duplication

The implementation acknowledges that CPU and GPU variants produce different results due to different underlying algorithms.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The changes are well-structured, follow existing patterns in the codebase (similar to curand_poisson_dist in randomizer.cuh), include comprehensive test coverage with histogram validation, and properly handle CPU/GPU code paths. The refactoring improves code maintainability by eliminating duplication.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
dali/operators/random/philox.h 5/5 Added result_type, min(), and max() to satisfy UniformRandomBitGenerator requirements for C++ standard library compatibility
dali/operators/random/random_dist.h 5/5 Added CurandGenerator wrapper and poisson_dist implementation using cuRAND for GPU and std::poisson_distribution for CPU; includes comprehensive documentation
dali/operators/random/random_dist_test.cu 5/5 Moved CurandGenerator to header file, refactored test utilities, added new Poisson distribution GPU tests with histogram validation

Sequence Diagram

sequenceDiagram
    participant Test as Test Code
    participant Philox as Philox4x32_10
    participant PoissonDist as poisson_dist
    participant CurandGen as CurandGenerator
    participant cuRAND as curand_poisson
    participant StdLib as std::poisson_distribution

    Note over Test,StdLib: GPU Path
    Test->>Philox: init(seed, seq, offset)
    Test->>CurandGen: CurandGenerator(curand_state)
    Test->>PoissonDist: operator()(CurandGenerator)
    PoissonDist->>CurandGen: Access state
    PoissonDist->>cuRAND: curand_poisson(&state, mean)
    cuRAND-->>PoissonDist: uint32_t value
    PoissonDist-->>Test: uint32_t value

    Note over Test,StdLib: CPU Path
    Test->>Philox: init(seed, seq, offset)
    Test->>PoissonDist: operator()(Philox)
    PoissonDist->>StdLib: std::poisson_distribution(mean)(rng)
    StdLib->>Philox: operator()() [multiple calls]
    Philox-->>StdLib: uint32_t random values
    StdLib-->>PoissonDist: uint32_t value
    PoissonDist-->>Test: uint32_t value
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

mzient and others added 6 commits November 25, 2025 13:50
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michał Zientkiewicz <[email protected]>
Signed-off-by: Michal Zientkiewicz <[email protected]>
Signed-off-by: Michal Zientkiewicz <[email protected]>
@mzient mzient force-pushed the poisson_dist_curand_std branch from 51d34e0 to e475f07 Compare November 25, 2025 14:11
Comment on lines -27 to -34
template <typename CurandState>
struct CurandGenerator {
CurandState &state;
__device__ explicit CurandGenerator(CurandState &s) : state(s) {}
__device__ inline uint32_t operator()() const {
return curand(&state);
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to non-tests.

}
}

namespace {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a header file.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Signed-off-by: Michal Zientkiewicz <[email protected]>
Signed-off-by: Michal Zientkiewicz <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Signed-off-by: Michal Zientkiewicz <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

namespace dali {
namespace random {

template <typename CurandState>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here from the tests. It'll be useful in production code.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [39149793]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [39149793]: BUILD FAILED

@rostan-t rostan-t self-assigned this Nov 26, 2025
@jantonguirao jantonguirao self-assigned this Nov 26, 2025
@mzient mzient merged commit a66380f into NVIDIA:main Nov 26, 2025
5 of 6 checks passed
mdabek-nvidia pushed a commit to mdabek-nvidia/DALI that referenced this pull request Nov 27, 2025
The distribution carries only the mean value and the actual distribution is selected a evaluation time.
The results are not consistent between CPU and GPU.
---------
Signed-off-by: Michal Zientkiewicz <[email protected]>
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.

4 participants