Remove static dimension type parameter#42
Remove static dimension type parameter#42projekter wants to merge 4 commits intoJuliaMath:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42 +/- ##
==========================================
+ Coverage 63.88% 66.95% +3.06%
==========================================
Files 1 1
Lines 108 115 +7
==========================================
+ Hits 69 77 +8
+ Misses 39 38 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Use length of x as dimension
|
Ok, how about this? The dimension is now grabbed from the vector as you suggested, and it can be any |
|
I'm thinking of |
src/Sobol.jl
Outdated
| new(SobolSeq(N), lb, ub) | ||
| function ScaledSobolSeq{T,X}(lb::Vector{T}, ub::Vector{T}) where {T,X<:AbstractVector{UInt32}} | ||
| length(lb)==length(ub) || throw(DimensionMismatch("lb and ub do not have same length")) | ||
| new{T,X}(SobolSeq{X}(length(lb)), lb, ub) |
There was a problem hiding this comment.
Maybe
| new{T,X}(SobolSeq{X}(length(lb)), lb, ub) | |
| x = similar(lb, T) # determine the type of array from lb | |
| new{T,typeof{x}}(x, lb, ub) |
There was a problem hiding this comment.
But if you want to have a much greater level of flexibility, then this is overly restrictive - the bounds could very well be immutable SVectors, but x would have to be its mutable complement.
|
Now you can explicitly pass any kind of vector to |
3 participants
Currently, the "dimension" of the Sobol sequence is implemented as a static type parameter. However, the whole code never makes any use that the value is known at compile time. Instead, all it does is iterating over
1:ndims(which will typically be not so small that loop unrolling would be favorable in any way, and the loop bodies are too long for this anyway) and creating vectors of lengthndims, which turns the static parameter into a runtime variable.I would say that the whole point why for example
Arrayuses a static dimension parameter instead of dynamic ones is:For multiple
SobolSeqof different dimension, instead, the compiler has to regenerate the code for every single method with the only difference between them being an integer constant in the function body, which is very inefficient. On top on this, any package that uses this one and needs to generate aSobolSeqwith only runtime-known length (actually, I'm here because ofMultistartOptimization.jl), unavoidably now has type instabilities and uninferrability (or it follows the same approach and "staticizes" the dynamical value, leading to dynamical dispatch and compiler overhead by method generation).Given that there is no technical reason or benefit for the static type parameter, I propose to change it to a field of the struct instead, which solves all the mentioned problems.
SobolSeqis now unparametrized andScaledSobolSeqonly contains the type of the boundaries as a type parameter.This is a breaking change if people explicitly wrote
SobolSeq{N}before, therefore this would be a major version increment.Note that I increased the Julia version to 1.8 since I declared the
ndimsfieldconst; however, this is the only reason for the compatibility reduction and can of course also be removed if you'd rather like to keep compatibility.Closes #18 as obsolete (see also my comment there).