-
Notifications
You must be signed in to change notification settings - Fork 16
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
Updated OperatorEnum to use any data type (not just Numbers) #85
Conversation
Benchmark Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good, nice work! I left some comments below.
Also, to make the formatter happy you can install JuliaFormatter
and then run this in the root of the repository:
julia --startup-file=no --threads=auto -O0 --compile=min -e 'using JuliaFormatter; format("."; verbose=true)'
and it should format all of the code so the formatting test passes. (The extra flags just make it startup faster; though aren't necessary).
By the way, I wonder if using a 0-dimensional array instead of a scalar could simplify some code? I'm not sure if it would or not (see bottom example for a caveat), but you could try it if interested. You can make 0-dimensional arrays with: julia> zeros(Float64)
0-dimensional Array{Float64, 0}:
0.0
julia> fill(0.0)
0-dimensional Array{Float64, 0}:
0.0
julia> x = Array{Float64}(undef); x .= 0.0; x
0-dimensional Array{Float64, 0}:
0.0 To index a zero-dimensional array, you use julia> x = zeros(Float64)
0-dimensional Array{Float64, 0}:
0.0
julia> x[] = 1.0
1.0
julia> x
0-dimensional Array{Float64, 0}:
1.0 Perhaps this simplifies some things like splatting because However there are some issues with zero-dimensional arrays... For example, julia> x = zeros(Float64)
0-dimensional Array{Float64, 0}:
0.0
julia> x .+ x
0.0
julia> x + x
0-dimensional Array{Float64, 0}:
0.0 which I find a bit weird, and might make other parts of the code more complex and unintuitive. So, completely up to you! I'm not sure it's worth it or not. Also, I think storing data in a zero-dimensional array may incur extra garbage collection cycles whereas a scalar might just get inlined. |
It seems like 0-dimensional arrays aren't used much so it might run into some weird behavior... Maybe just better to stick with your existing approach of scalars. |
Co-authored-by: Miles Cranmer <[email protected]>
Co-authored-by: Miles Cranmer <[email protected]>
I think the line causing an error: @test_throws ErrorException ex(randn(1, 1)) can just be removed. This line just asserts that (up to this point) you couldn't use this type of behavior. But now you can! So that check can be turned off. |
Also I wonder if |
struct Max2Tensor{T} | ||
dims::UInt8 # number of dimmentions | ||
scalar::T | ||
vector::Vector{T} | ||
matrix::Matrix{T} | ||
Max2Tensor{T}() where {T} = new(Int8(0), zero(T), T[], Array{T,2}(undef, 0, 0)) | ||
function Max2Tensor{T}(scalar::W) where {T,W<:Number} | ||
return new(Int8(0), Base.convert(T, scalar), T[], Array{T,2}(undef, 0, 0)) | ||
end | ||
function Max2Tensor{T}(vector::Vector{W}) where {T,W<:Number} | ||
return new(Int8(1), zero(T), Vector{T}(vector), Array{T,2}(undef, 0, 0)) | ||
end | ||
function Max2Tensor{T}(matrix::Matrix{W}) where {T,W<:Number} | ||
return new(Int8(2), zero(T), T[], Matrix{T}(matrix)) | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you are still planning to go in this direction (from the other struct you showed me) but just an idea: you could write this as:
julia> struct MultiTensor{T,N,A<:Tuple{Vararg{<:Any,N}}}
dims::UInt8
data::A
function MultiTensor{T,N}() where {T,N}
@assert N >= 1
data = ntuple(i -> i == 1 ? zero(T) : zeros(T, ntuple(_ -> 0, Val(i-1))), Val(N))
return new{T,N,typeof(data)}(zero(UInt8), data)
end
end
For example, for N=3
, this would expand to:
function MultiTensor{T,N}() where {T,N}
data = (zero(T), zeros(T, 0), zeros(T, 0, 0))
return new{T,N,typeof(data)}(zero(UInt8), data)
end
and (I think) be type stable too, because ntuple(..., Val(n))
will expand to exactly n
-tuple if n
is known to the compiler.
Then you can access the data with val.data[1]
for scalars, val.data[2]
for vectors, and so on.
(But I'm assuming you are going the other way with the fixed array?)
Pull Request Test Coverage Report for Build 9806989095Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9821555509Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9822135489Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9822262239Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9822341017Details
💛 - Coveralls |
d04d349
to
0c8aab9
Compare
Pull Request Test Coverage Report for Build 9828697768Details
💛 - Coveralls |
There are some issues with defining |
Pull Request Test Coverage Report for Build 9828915053Details
💛 - Coveralls |
Changes:
ValueInterface
module. If you want to use anOperatorEnum
on a specific data type, you have to implement all the functions from that module.on_type
argument to@extend_operators
, which restricts the type parameter ofAbstractNode
s that have the operators defined on them.