Skip to content

add minimalvec#22

Merged
Jutho merged 4 commits into
mainfrom
jh/addminimalvec
Nov 14, 2024
Merged

add minimalvec#22
Jutho merged 4 commits into
mainfrom
jh/addminimalvec

Conversation

@Jutho
Copy link
Copy Markdown
Member

@Jutho Jutho commented Nov 13, 2024

No description provided.

@Jutho
Copy link
Copy Markdown
Member Author

Jutho commented Nov 14, 2024

Ok I think this is ready. Maybe some review is welcome, @lkdvos ?

Comment thread test/minimalmvec.jl
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, this is quite an extensive testset for something that is simply wrapping Vector. We could simply have:

@testset "scale" begin
    a = rand(2)
    alfa = rand()
    @test scale(a, alfa) == scale(MinimalMVec(a), alfa).vec 
    ...
end

Along with some simple checks of type stability. I am also fine with just leaving this as is, but less code is easier to maintain 😉

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, it's copy pasted code, with small adjustments for the specific case.

It did help me with finding some problem in our fallbacks though. Base has a mul!(::Any, :Any, :Any) method, so testing applicable(mul!, x, y, alpha) was kind of pointless. That Tuple{Any,Any,Any} method assumes this is a matrix multiplication call, and just adds 2 more arguments true and false playing the role of alpha and beta. But that is of course pointless if you want to do what we call scale! or add!, and alpha is already the third argument. I guess this just illustrates what I dislike about the current mul! method and why I started VectorInterface.jl in the first place.

It actually also helped me track down another error in our MinimalVec implementation, where our interface promises that scale!!(y, x, alpha) will always work. So even if y is mutable, but happens to have a scalar type that is not compatible with the scalar type of x * alpha, the method should not complain and return a new vector (which actually has scalar type that is the promotion of that of y, x and alpha)`. This was broken and resulting in an error in the original implementation.

So I would favor keeping it for now.

Comment thread test/minimalmvec.jl Outdated
Comment thread test/minimalsvec.jl Outdated
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
@Jutho Jutho merged commit eceb575 into main Nov 14, 2024
@lkdvos lkdvos deleted the jh/addminimalvec branch November 14, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants