Updated OperatorEnum to use any data type (not just Numbers) #85
Conversation
This file contains 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
Benchmark Results
|
There was a problem hiding this comment.
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.0To 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.0Perhaps 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.0which 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 |
MilesCranmer
reviewed
Jul 3, 2024
MilesCranmer
reviewed
Jul 4, 2024
Comment on lines
13
to
29
| 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.
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
endFor 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)
endand (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 |
MilesCranmer
force-pushed
the
generic-update
branch
from
d04d349
to
0c8aab9
Compare
Pull Request Test Coverage Report for Build 9828697768Details
💛 - Coveralls |
|
There are some issues with defining |
MilesCranmer
approved these changes
Jul 7, 2024
Pull Request Test Coverage Report for Build 9828915053Details
💛 - Coveralls |
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.
Changes:
ValueInterfacemodule. If you want to use anOperatorEnumon a specific data type, you have to implement all the functions from that module.on_typeargument to@extend_operators, which restricts the type parameter ofAbstractNodes that have the operators defined on them.