Skip to content
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

add minimalvec #22

Merged
merged 4 commits into from
Nov 14, 2024
Merged

add minimalvec #22

merged 4 commits into from
Nov 14, 2024

Conversation

Jutho
Copy link
Owner

@Jutho Jutho commented Nov 13, 2024

No description provided.

@Jutho
Copy link
Owner Author

Jutho commented Nov 14, 2024

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

Copy link
Collaborator

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
Owner 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.

test/minimalmvec.jl Outdated Show resolved Hide resolved
test/minimalsvec.jl Outdated Show resolved Hide resolved
Co-authored-by: Lukas Devos <[email protected]>
@Jutho Jutho merged commit eceb575 into main Nov 14, 2024
6 checks passed
@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