Adding a Wigner matrix initialiser for #284#412
Adding a Wigner matrix initialiser for #284#412am-1118 wants to merge 2 commits intoSciML:masterfrom
Conversation
…n of the function after feedback
src/inits/inits_reservoir.jl
Outdated
| for j in 1 : res_size | ||
| if i==j | ||
| # Diagonal element | ||
| W[i, j] = randn(rng, T) * T(sqrt(2)) * T(std) |
There was a problem hiding this comment.
randn(rng, T) sohuld be taken from DeviceAgnostic
also I don't understand why T(sqrt(2))
There was a problem hiding this comment.
For the standard Wigner matrix, T(sqrt(2)) is for the diagonal elements as they are sampled from N(0, 2*variance), which implies sqrt(2) * std. I will modify the arguments to accept two arbitrary std values for the diagonal and off-diagonal distributions, as described in the issue. Modified rand -> DeviceAgnostic.rand
src/inits/inits_reservoir.jl
Outdated
| end | ||
|
|
||
| # Make the matrix symmetric | ||
| W = Symmetric(W) |
There was a problem hiding this comment.
add this as a kwarg option that the user can choose return_symmetric::Bool = false
There was a problem hiding this comment.
also need to import this from LinearAlgebra here
There was a problem hiding this comment.
I've added the return_symmetric argument, but I've kept the default case as true, which the user can set to false.
using LinearAlgebra: eigvals, I, qr, Diagonal, diag, mul!
Should I paste the above line into the inits_reservoir.jl code?
src/inits/inits_reservoir.jl
Outdated
| W = scale_radius!(W, T(radius)) | ||
|
|
||
| # 5. Check for NaN or Inf values | ||
| check_inf_nan(W) |
There was a problem hiding this comment.
I don't think this is necessary, this issue usually arises from the sparsity
There was a problem hiding this comment.
Should I remove that line ? Or add some other check?
src/inits/inits_reservoir.jl
Outdated
| res_size = dims[1] | ||
|
|
||
| # 2. Initialise the empty matrix using the requested type T | ||
| W = zeros(T, res_size, res_size) |
There was a problem hiding this comment.
try to avoid single letter naming if possible W -> reservoir_matrix. follow the conventions of the other initializers so that it makes it easier to search for stuff in the file
3 participants
This PR implements the Wigner matrix initialisation to resolve Issue #284.
Changes Made
wigner_initto the matrix initialisers.LinearAlgebra.Symmetricto make the matrix symmetric.check_res_size,scale_radius!, andcheck_inf_nanutilities.@testsetblock to verify reservoir dimensions, data type, and reservoir symmetry.