Repository navigation
(feat): rand! / randn! — random-array pool constructors (CPU + Metal + CUDA) - #43
Merged
Merged
Conversation
Add rand!/randn! convenience constructors as peers of zeros!/ones!,
extending Random.rand!/randn! with pool-backed methods. They are
re-exported (same binding), so `using AdaptiveArrayPools` exposes them
with no conflict alongside `using Random`.
- Random added as a hard dependency (stdlib, ~0 load cost)
- Forms: rand!(pool, [T,] dims...) and the collection/range sampling form
rand!(pool, S, dims...) (e.g. rand!(pool, 1:6, n)); randn! for N(0,1)
- Wired into @with_pool at all three sites (type pre-registration,
call->_impl! transform, escape detection). The collection form
self-records its eltype touch since the macro cannot pre-register
eltype(S)
- DisabledPool{:cpu} fallbacks; CPU-only (GPU parity deferred)
- Seed-equivalent with plain rand/randn: pool arrays are genuine Arrays,
so RNG consumption matches exactly
Tests (test/test_random.jl): seed-equivalence incl. consecutive-draw and
@with_pool RNG-neutrality, zero-allocation, type-stability (@inferred),
@maybe_with_pool fallback, escape detection, and DisabledPool.
The live GPU pool inherits the core _rand_impl!/_randn_impl! (which fill the
acquired CuArray/MtlArray via Random.rand!/randn!, run on-device by CUDA.jl /
Metal.jl) — verified on Metal for Float32/Float16/Int32. Each extension adds:
- DisabledPool{:cuda}/{:metal} rand!/randn! fallbacks (CUDA.rand/randn,
Metal.rand/randn)
- A clear ArgumentError for the collection/range form rand!(pool, S, dims):
GPU RNGs have no arbitrary-collection sampler, so it is CPU-only. Overriding
_rand_impl! covers both the @with_pool macro path and the direct path, for
live and disabled pools.
Tests: test/metal/test_random.jl (34 pass locally) and test/cuda/test_random.jl
(mirror, to run on a CUDA machine).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43 +/- ##
==========================================
+ Coverage 95.48% 95.61% +0.13%
==========================================
Files 14 14
Lines 3232 3309 +77
==========================================
+ Hits 3086 3164 +78
+ Misses 146 145 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds pooled random-array constructors rand! / randn! as peers of zeros! / ones!, integrating them into @with_pool (type extraction, call transformation, escape detection) and providing CPU + Metal + CUDA behavior (including explicit GPU rejection of collection/range sampling).
Changes:
- Extend and re-export
Random.rand!/Random.randn!withAbstractArrayPoolmethods, plus_rand_impl!/_randn_impl!for macro-transformed calls. - Add CPU + Metal + CUDA tests covering RNG stream equivalence,
@with_poolbehavior, DisabledPool fallbacks, and GPU collection-form rejection. - Update package deps/docs/README to include Random and document the new convenience functions.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_random.jl | New comprehensive CPU test coverage for pooled rand!/randn! semantics and RNG equivalence. |
| test/runtests.jl | Includes the new CPU random-constructor test file in the suite. |
| test/Project.toml | Adds Random to test dependencies. |
| test/metal/test_random.jl | New Metal backend tests for typed/default GPU random fills + rejection of collection sampling. |
| test/metal/runtests.jl | Includes the new Metal random-constructor tests. |
| test/cuda/test_random.jl | New CUDA backend tests mirroring Metal coverage (to run on CUDA-capable hosts). |
| test/cuda/runtests.jl | Includes the new CUDA random-constructor tests. |
| src/macros.jl | Registers rand!/randn! in type extraction, call→impl transforms, and escape-detection sets. |
| src/convenience.jl | Implements pooled rand!/randn!, internal impls, and DisabledPool CPU fallbacks. |
| src/AdaptiveArrayPools.jl | Imports/re-exports Random.rand!/randn! and exports them as part of the public API. |
| README.md | Documents rand!/randn! in the convenience-function table and explains RNG behavior. |
| Project.toml | Adds Random to [deps] and [compat]. |
| ext/AdaptiveArrayPoolsMetalExt/convenience.jl | Adds Metal DisabledPool fallbacks and GPU-side collection-form rejection via _rand_impl! override. |
| ext/AdaptiveArrayPoolsCUDAExt/convenience.jl | Adds CUDA DisabledPool fallbacks and GPU-side collection-form rejection via _rand_impl! override. |
| docs/design/rand_randn_pool_constructors.md | New design doc describing the feature and rationale (currently contains a few stale statements). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/cuda/test_extension.jl simulated a @with_pool scope by setting pool._current_depth = 2 directly, but skipped the lockstep growth of the per-depth _touched_has_others vector that a real checkpoint! performs. get_typed_pool! then wrote _touched_has_others[2] into a 1-element vector — masked by @inbounds normally, but a BoundsError under Pkg.test's --check-bounds=yes (seen on NERSC/CI). Use the real checkpoint!(pool), exactly as the Metal test already did, and assert _touched_has_others == [false, true] in both the CUDA and Metal tests so the depth flag is verified rather than just executed. Test-only change.
Codecov flagged 13 uncovered patch lines — all never-called method bodies: - convenience.jl: randn! typed-NTuple public, the two _randn_impl! NTuple delegations (reached via the macro size(x) transform), and the rand!/randn! DisabledPool NTuple fallbacks - macros.jl: the qualified-name AdaptiveArrayPools.rand!/randn! → _impl! transform branches Add two testsets exercising every NTuple form (direct + size(x) macro path + DisabledPool) and a qualified-name call inside @with_pool. All 13 lines now hit. Also refresh the design doc: status -> Implemented, GPU-parity row -> done (addresses the stale-doc points from the Copilot PR review).
docs/design/rand_randn_pool_constructors.md is an internal/local working note, not part of the shipped package. Remove it from version control (kept locally, untracked) so it no longer appears in the PR.
This file contains hidden or 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
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.
Summary
Adds
rand!/randn!as pooled peers ofzeros!/ones!.They extend
Random.rand!/Random.randn!with methods on the pool type and are re-exportedAPI
GPU (Metal + CUDA)
Live GPU pools inherit the typed/default forms for free — the core impl fills the acquired
CuArray/MtlArrayviaRandom.rand!/randn!on-device, same mechanism aszeros!(verified on Metal).The collection/range form is CPU-only (no GPU sampler) and raises a
clear error; each extension adds only
DisabledPoolfallbacks plus that guard.Out of scope
GPU collection sampling ·
randexp!·rng-argument forms ·rand!(pool, Bit, …).Note
Fixed a 'previously' broken tests in CUDA runtets.