-
Notifications
You must be signed in to change notification settings - Fork 9
Add elementary matrices constructors #20
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
===========================================
- Coverage 82.10% 67.82% -14.28%
===========================================
Files 3 4 +1
Lines 95 115 +20
===========================================
Hits 78 78
- Misses 17 37 +20
Continue to review full report at Codecov.
|
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.
Very nice. This is a first round of review before we can do more optimizations. I would also review the index style IndexLinear() is what we want? I have to review the interface myself, but I thought that Cartesian index was desired.
After you incorporate the suggestions and review the index style, please go ahead and add some tests. That we can see it in action.
src/matrices.jl
Outdated
UnitMatrix{T}(d) | ||
UnitColumn{T}(d) |
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.
Let's rename these to OnesMatrix
and OnesColumn
to remove ambiguity with the unit element.
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.
Actually, let's simply call it JMatrix
and JColumn
to match the notation. Otherwise we will have a difficult time finding good names for the other matrices below.
src/matrices.jl
Outdated
F{T}(d) | ||
G{T}(N) | ||
H{T}(d) |
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.
Let's rename these to FMatrix
, GMatrix
and HMatrix
. And then we create an alias F, G, H
for the type and export it. See the I example in the standard library LinearAlgebra.
src/matrices.jl
Outdated
Elementary matrices as defined by Aitchison 1986. | ||
|
||
## Examples | ||
|
||
```julia | ||
julia> UnitMatrix(5) | ||
julia> UnitColumn{Int}(2) | ||
julia> G(4) | ||
julia> H{Float64}(2) | ||
``` | ||
|
||
When the type of the elements isn't explicitly defined, `Float64` is used. | ||
""" |
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.
Notice that docstrings apply to a specific struct or function. Please split this string into multiple docstrings right before the structs defining the matrices. Also remove the leading empty line between the string and the struct so that Julia doc system can find the association.
src/matrices.jl
Outdated
struct UnitColumn{T} <: AbstractArray{T, 1} | ||
d::Int | ||
end | ||
UnitColumn(d::Int) = UnitColumn{Float64}(d) |
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.
We should default to Bool because then Julia will use its promote rules with other types when needed. If you specify Float64 you are already forcing double precision and inhibiting some applications (ex: GPU with Float32).
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.
Since my knowledge of the Julia's compiler is limited, I'd like to document a few bullet points that influenced my decision to use Float64 as a default:
-
I saw that the Identity Matrix was implemented using Bool by default, but my idea was to avoid casting during the operations, which can hinder AVX's performance.
-
We need to also keep in mind that the auto casting is non-associative and non-commutative, ie,
julia> typeof(Float32(1) * true/( true + true ))
Float32
julia> typeof(Float32(1) * (true/( true + true )))
Float64
julia> typeof( true/( true + true ) * Float32(1))
Float64
which means that auto casting at compilation time probably isn't possible for all cases in Julia.
- The default promotions in Julia are to Int64 and to Float64, so for GPU(s) I'd sitck to manual casting.
With those bullet points in mind, I see no problem in switching to Bool and Int whenever possible.
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.
- I saw that the Identity Matrix was implemented using Bool by default, but my idea was to avoid casting during the operations, which can hinder AVX's performance.
For some of these special matrices we will optimize the matrix-vector multiplication in a way that doesn't even use the type stored in the struct. For example, multiplying the matrix J
by any vector x
is equivalent to producing a new vector where the entries are sum(x)
. That is the beauty of Julia. If you read the entire LinearAlgebra example with the identity matrix, you will see that multiplications and many other operations are completely erased at compile time. We will do the same here after the basic struct is ready for tests.
src/matrices.jl
Outdated
UnitColumn(d::Int) = UnitColumn{Float64}(d) | ||
Base.size(j::UnitColumn) = (j.d,) | ||
Base.IndexStyle(::Type{UnitColumn}) = IndexLinear() | ||
Base.getindex(j::UnitColumn, i::Int) = convert(eltype(j), 1) |
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.
Base.getindex(j::UnitColumn, i::Int) = convert(eltype(j), 1) | |
Base.getindex(j::UnitColumn{T}, i) where {T} = one(T) |
src/matrices.jl
Outdated
Base.getindex(J::UnitMatrix, i::Int, j::Int) = convert(eltype(J), 1) | ||
|
||
|
||
struct UnitColumn{T} <: AbstractArray{T, 1} |
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.
struct UnitColumn{T} <: AbstractArray{T, 1} | |
struct UnitColumn{T} <: AbstractVector{T} |
src/matrices.jl
Outdated
F(d::Int) = F{Float64}(d) | ||
Base.size(A::F) = (A.d, A.d+1) | ||
Base.IndexStyle(::Type{F}) = IndexLinear() | ||
Base.getindex(A::F, i::Int, j::Int) = convert(eltype(A), ifelse(i==j, 1, 0) - (j==(A.d+1))) |
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.
Instead of using convert(eltype(A), x)
you can use one(T)
and zero(T)
to get the unit and zero elements for the type T. This type T can be specified in the argument as A::F{T} where {T}
. See the other suggestion above for a concrete example.
src/matrices.jl
Outdated
Base.getindex(A::F, i::Int, j::Int) = convert(eltype(A), ifelse(i==j, 1, 0) - (j==(A.d+1))) | ||
|
||
struct G{T} <: AbstractArray{T, 2} | ||
N::Int |
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.
Let's use 2 white spaces for indentation to follow the existing convention in the source.
src/matrices.jl
Outdated
G(N::Int) = G{Float64}(N) | ||
Base.size(A::G) = (A.N, A.N) | ||
Base.IndexStyle(::Type{G}) = IndexLinear() | ||
Base.getindex(A::G, i::Int, j::Int) = convert(eltype(A), ifelse(i==j, 1, 0) - 1.0/A.N) |
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.
Similar comment applies, you can use one(T)
and zero(T)
to simplify the expression. Also, you can remove the specification i::Int
and just type i
. That way people can index with other kinds of integers.
src/matrices.jl
Outdated
H(d::Int) = H{Float64}(d) | ||
Base.size(A::H) = (A.d, A.d) | ||
Base.IndexStyle(::Type{H}) = IndexLinear() | ||
Base.getindex(A::H, i::Int, j::Int) = convert(eltype(A), (ifelse(i==j, 2, 1))) |
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.
Same comment applies.
Very good excerpts from the docs, thanks. Would you agree that the choice of index style is a function of how we intend to use these arrays in typical workflows? I am assuming that most algorithms that we will implement with these matrices will be expressed in terms of Cartesian indices. If we choose |
We (now) agree on all of the suggestions. I've done a few benchmarks (attached Pluto notebook), and the conclusion is that choosing any of the styles doesn't affect the performance very much. However, both |
The results of your benchmark are very different for me. I would not use Pluto for that purpose. Please use a raw Julia script and use the BenchmarkTools.jl package to assess the distribution of times. All the cells of the notebook are around 100ms for me on the following machine: julia> versioninfo()
Julia Version 1.6.2
Commit 1b93d53fc4 (2021-07-14 15:36 UTC)
Platform Info:
OS: Linux (x86_64-pc-linux-gnu)
CPU: Intel(R) Core(TM) i7-9700 CPU @ 3.00GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-11.0.1 (ORCJIT, skylake)
Environment:
JULIA_NUM_THREADS = 8 Also, you tend to focus too much on performance prematurely. Let's get the core functionality first, and then later we can optimize the code for performance. |
My suggestion is to first fix the name of the structs as suggested, then use IndexCartesian style for matrices and IndexLinear style for columns. After that, I suggest writing tests with basic matrix multiplications and identities from the chapters of the book. Only then, after everything is working we can start thinking about performance. |
Pluto isn't the problem. Apparently it's a known bug, fixed at the end of 2020: a huge overhead when using I was using Ubuntu 21.04, with the default Julia version (1.5.3). Considering that this is a non LTS version of Ubuntu, it's likely that most non rolling-release distros will have similar performance problems in their package managed versions of Julia. While I agree that early optimization is a pretty bad idea, a few sanity performance checks can be pretty useful: both to catch bugs and to check if the compilers/interpreters are doing anything unusual. |
Pluto isn't the right place to share benchmarks. I had to open a Pluto session just to reproduce the times, which btw vary in multiple runs of the notebook. As I mentioned, the correct way to benchmark code is BenchmarkTools.jl, the @benchmark macro. It is much more reliable than looking at Pluto cell times.
And we keep discussing issues that are irrelevant for the task at hand. As you can see, the current version of the PR is unmergeable, it doesn't have basic tests, and we are discussing performance on a specific Ubuntu distribution with an old release of the language...
Do you agree that this is not the case here? You are trying to optimize something that is extremely low-level and likely irrelevant at this point in time. I agree with you that it is good to learn and be skilled in this low-level optimizations, it is just not the right time. I urge you to fight this tendency and focus on the task. That way we will be more efficient together, and I am sure we will do wonderful things. There will be lots of opportunity to investigate performance if that is something that is blocking our work. |
Is there anything that I can do to help you in the process? What about writing down a list of steps you plan to achieve to clarify priorities? I always do that and it helps me a lot. |
And just to be clear, I really appreciate your skills. It is not easy to find someone with such great knowledge of performance optimizations and attention to low-level details. I strongly believe that you will outperform yourself by a lot with very minor tweaks here and there. Please don't bother if my comments sound harsh sometimes, I am doing it with the best intentions. |
I'm learning a lot with the comments, and I appreciate the patience of looking things thoroughly. :) |
Looking forward to growing together 💯 Let me know when you have an updated PR ready for review. |
5f56023
to
dadceb1
Compare
As defined by Aitchison 1986.
dadceb1
to
089918c
Compare
As defined by Aitchison 1986.