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

Code suggestions for #1210 #1213

Conversation

hyrodium
Copy link
Collaborator

@hyrodium hyrodium commented Oct 29, 2023

Overview

This PR adds suggestions for #1210.

  • Add support for @SArray rand(rng, 3)
  • Simple output for @macroexpand @SArray rand(rng, Float32, 4)
  • Better error handling for @SArray rand(rng, Float32, Float32)
  • Add support for randn and randexp

Details

Add support for @SArray rand(rng, 3)

On the original PR #1210

julia> using Random, StaticArrays

julia> rng = Random.MersenneTwister(42)
MersenneTwister(42)

julia> @SArray rand(rng, 3)
ERROR: MethodError: Cannot `convert` an object of type Vector{Float64} to an object of type Int64
[...]

On this PR

julia> using Random, StaticArrays

julia> rng = Random.MersenneTwister(42)
MersenneTwister(42)

julia> @SArray rand(rng, 3)
3-element SVector{3, Float64} with indices SOneTo(3):
 0.5331830160438613
 0.4540291355871424
 0.017686826714964354

Simple output for @macroexpand @SArray rand(rng, Float32, 4)

On the original PR #1210

julia> @SArray rand(rng, Float32, 4)
4-element SVector{4, Float32} with indices SOneTo(4):
 0.9603034
 0.76554394
 0.46937835
 0.2332462

julia> @macroexpand @SArray rand(rng, Float32, 4)
quote
    #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:214 =#
    if rng isa StaticArrays.DataType
        #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:215 =#
        StaticArrays.rand((SArray){(Tuple){Float32, 4}, rng})
    elseif #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:216 =# rng isa StaticArrays.Integer
        #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:217 =#
        StaticArrays.rand((SArray){(Tuple){rng, Float32, 4}})
    elseif #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:218 =# rng isa (StaticArrays.Random).AbstractRNG
        #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:220 =#
        (StaticArrays.StaticArrays)._rand(rng, Float32, StaticArrays.Size(4), (SArray){StaticArrays.Tuple{4}, (StaticArrays.Random).gentype(Float32)})
    else
        #= /home/hyrodium/.julia/dev/StaticArrays/src/SArray.jl:231 =#
        (StaticArrays.StaticArrays)._rand((StaticArrays.Random).GLOBAL_RNG, rng, StaticArrays.Size(Float32, 4), (SArray){StaticArrays.Tuple{Float32, 4}, (StaticArrays.Random).gentype(rng)})
    end
end

On this PR

julia> @SArray rand(rng, Float32, 4)
4-element SVector{4, Float32} with indices SOneTo(4):
 0.22553396
 0.18603194
 0.16922617
 0.9104786

julia> @macroexpand @SArray rand(rng, Float32, 4)
:(StaticArrays._rand_with_Val(SArray, rng, Float32, StaticArrays._int2val(rng), StaticArrays._int2val(Float32), StaticArrays.Val(StaticArrays.tuple(4))))

Add support for randn and randexp

On the original PR #1210

julia> @SArray randn(rng, 5)
ERROR: TypeError: in Tuple, in parameter, expected Type, got a value of type MersenneTwister
Stacktrace:
[...]

julia> @SArray randexp(rng, 5)
ERROR: TypeError: in Tuple, in parameter, expected Type, got a value of type MersenneTwister
Stacktrace:
[...]

On this PR

julia> @SArray randn(rng, 5)
5-element SVector{5, Float64} with indices SOneTo(5):
  0.18702790710363
  0.5181487878771377
  1.4913791170403063
  0.3675627461748204
 -0.8862052960481365

julia> @SArray randexp(rng, 5)
5-element SVector{5, Float64} with indices SOneTo(5):
 1.11449101941971
 1.473948306820646
 0.06666365902783734
 0.39638956977933426
 0.5090311768294857

Note

Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I think this is a good idea, thanks for proposing that. Could you add some tests for the new functionality and safeguards for ex.args indexing (like the one I've proposed)?

src/SArray.jl Outdated Show resolved Hide resolved
@hyrodium
Copy link
Collaborator Author

Should I update @SVector and @SMatrix too? (x-ref: #1214)

@mateuszbaran
Copy link
Collaborator

Yes, I think it would be good to update these two macros too.

@hyrodium
Copy link
Collaborator Author

Hmm, I'm thinking of reusing static_array_gen in @SVector macro etc., but that may takes a while...
Replacing static_vector_gen in @SVector definition with

macro SVector(ex)
    ex = static_array_gen(SArray, ex, __module__)
    :(SVector($ex))
end

is almost fine, but some tests fail..

@hyrodium
Copy link
Collaborator Author

I have updated static_vector_gen. I will update static_matrix_gen and tests tomorrow.

@hyrodium
Copy link
Collaborator Author

hyrodium commented Nov 1, 2023

Oooh, I didn't know that n=3; @SVector rand(n) is supported.

julia> n=3; @SVector rand(n)  # current master branch
3-element SVector{3, Float64} with indices SOneTo(3):
 0.41421419554720196
 0.8417028007187902
 0.05502353350873945

I assumed all of the array sizes must be integers but not variables in this PR.

julia> n=3; @SVector rand(n)  # on this PR
ERROR: LoadError: @SVector got bad expression: rand(n)
Stacktrace:
[...]

I noticed this behavior when testing @SMatrix...

for n=1:4
m = @SMatrix randn(n,n)

@mateuszbaran
Copy link
Collaborator

Yes, that's why some macro variants generate if statements.

@hyrodium
Copy link
Collaborator Author

hyrodium commented Nov 1, 2023

I'll update this PR this weekend:muscle:

@hyrodium hyrodium marked this pull request as draft November 12, 2023 11:42
@hyrodium hyrodium marked this pull request as ready for review January 2, 2024 13:24
@hyrodium
Copy link
Collaborator Author

hyrodium commented Jan 2, 2024

Sorry for the late. I think this is ready to review after CI results turn green!
I have also updated the PR description.

@mateuszbaran
Copy link
Collaborator

Thanks! I will check this PR.

Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

It looks great, thanks!

@mateuszbaran mateuszbaran merged commit 80f5136 into JuliaArrays:mbaran/fix-rand-generator Jan 2, 2024
22 of 29 checks passed
@hyrodium
Copy link
Collaborator Author

hyrodium commented Jan 3, 2024

Thanks for the quick review! 😄

mateuszbaran added a commit that referenced this pull request Jan 3, 2024
* Extend macros with rand to support custom samplers

* Fix tests

* Update Project.toml

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update test/arraymath.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update src/SVector.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update src/SMatrix.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update `SArray` macro

* fix reported issue; support rng in SArray and SMatrix

* Code suggestions for #1210 (#1213)

* move `ex.args[2] isa Integer`

* split `if` block

* simplify :zeros and :ones

* refactor :rand

* refactor :randn and :randexp

* update comments

* add _isnonnegvec

* update with `_isnonnegvec`

* add `_isnonnegvec(args, n)` method to check the size of `args`

* fix `@SArray` for `@SArray rand(rng,T,dim)` etc.

* update comments

* update `@SVector` macro

* update `@SMatrix`

* update `@SVector`

* update `@SArray`

* introduce `fargs` variable

* avoid `_isnonnegvec` in `static_matrix_gen`

* avoid `_isnonnegvec` in `static_vector_gen`

* remove unnecessary `_isnonnegvec`

* add `_rng()` function

* update tests on `@SVector` macro

* update tests on `@MVector` macro

* organize test/MMatrix.jl and test/SMatrix.jl

* organize test/MMatrix.jl and test/SMatrix.jl

* update with broken tests

* organize test/MMatrix.jl and test/SMatrix.jl for `rand*` functions

* fix around `broken` key for `@test` macro

* fix zero-length tests

* update `test/SArray.jl` to match `test/MArray.jl`

* update tests for `@SArray ones` etc.

* add supports for `@SArray ones(3-1,2)` etc.

* move block for `fill`

* update macro `@SArray rand(rng,2,3)` to use ordinary dispatches

* update around `@SArray randn` etc.

* remove unnecessary dollars

* simplify `@SArray fill`

* add `@testset "expand_error"`

* update tests for `@SArray rand(...)` etc.

* fix bug in `rand*_with_Val`

* cleanup tests

* update macro `@SMatrix rand(rng,2,3)` to use ordinary dispatches

* update macro `@SVector rand(rng,3)` to use ordinary dispatches

* move block for `fill`

* simplify `_randexp_with_Val`

---------

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>
avik-pal pushed a commit to avik-pal/StaticArrays.jl that referenced this pull request Jan 12, 2024
* Extend macros with rand to support custom samplers

* Fix tests

* Update Project.toml

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update test/arraymath.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update src/SVector.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update src/SMatrix.jl

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>

* Update `SArray` macro

* fix reported issue; support rng in SArray and SMatrix

* Code suggestions for JuliaArrays#1210 (JuliaArrays#1213)

* move `ex.args[2] isa Integer`

* split `if` block

* simplify :zeros and :ones

* refactor :rand

* refactor :randn and :randexp

* update comments

* add _isnonnegvec

* update with `_isnonnegvec`

* add `_isnonnegvec(args, n)` method to check the size of `args`

* fix `@SArray` for `@SArray rand(rng,T,dim)` etc.

* update comments

* update `@SVector` macro

* update `@SMatrix`

* update `@SVector`

* update `@SArray`

* introduce `fargs` variable

* avoid `_isnonnegvec` in `static_matrix_gen`

* avoid `_isnonnegvec` in `static_vector_gen`

* remove unnecessary `_isnonnegvec`

* add `_rng()` function

* update tests on `@SVector` macro

* update tests on `@MVector` macro

* organize test/MMatrix.jl and test/SMatrix.jl

* organize test/MMatrix.jl and test/SMatrix.jl

* update with broken tests

* organize test/MMatrix.jl and test/SMatrix.jl for `rand*` functions

* fix around `broken` key for `@test` macro

* fix zero-length tests

* update `test/SArray.jl` to match `test/MArray.jl`

* update tests for `@SArray ones` etc.

* add supports for `@SArray ones(3-1,2)` etc.

* move block for `fill`

* update macro `@SArray rand(rng,2,3)` to use ordinary dispatches

* update around `@SArray randn` etc.

* remove unnecessary dollars

* simplify `@SArray fill`

* add `@testset "expand_error"`

* update tests for `@SArray rand(...)` etc.

* fix bug in `rand*_with_Val`

* cleanup tests

* update macro `@SMatrix rand(rng,2,3)` to use ordinary dispatches

* update macro `@SVector rand(rng,3)` to use ordinary dispatches

* move block for `fill`

* simplify `_randexp_with_Val`

---------

Co-authored-by: Yuto Horikawa <hyrodium@gmail.com>
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.

2 participants