search

Jutho / KrylovKit.jl

Krylov methods for linear problems, eigenvalues, singular values and matrix functions
Other
284 stars 37 forks source link

is *(::Int, ::YourVector) a required operation #33

Closed rveltz closed 2 years ago

rveltz commented 4 years ago

Hi,

I am facing some issues because of those, etc. Should you not use mul,... to comply with the required methods for the custom vector type (which I pass) as described here?

rveltz commented 4 years ago

I also have this:

copyto!(::RecursiveVec{Array{Array{Float64,1},1}}, ::RecursiveVec{Array{Array{Float64,1},1}})

not defined. All this seems to happen on 0.5.0

Jutho commented 4 years ago

Yes it seems the docs were not updated, something with the build phase. However, the README contains the most important breaking change. You now need to define *(::Number, ::YourVectorType), but no longer eltype and similar(::YourVectorType,::Type{<:Number}).

However, the copyto! should not have happened. This I will need to fix.

Jutho commented 4 years ago

@rveltz, is the absence of copyto! being complained about by KrylovKit? Or in your linear operator that you wrote for it? Normally all methods should work without copyto!. See minimalvec.jl in the tests to see the minimal set of methods that need to be implemented for KrylovKit.jl to work. My apologies for breaking your code.

rveltz commented 4 years ago

It was me using copyto! on RecursiveVec...

No worries for the changes,

Bests

Jutho commented 4 years ago

Yes but I should not have deleted that. I did not quite know how to properly deprecate or transition the old required implementation into the new one, and so just implemented it as a breaking change. But for RecursiveVec, there is no reason that it should only have the minimal required interface. It was perfectly fine to keep copyto!, or at least deprecate it (but it can in fact stay). I will restore that behaviour.

I think I removed copyto! for RecursiveVec as a means of testing that it was not needed, before I added the MinimalVec implementation in the tests the minimal API more thoroughly. And I then forgot to restore copyto!.

rveltz commented 4 years ago

I think it is fine the way you did. It forces the user to comply with the new interface.

You can perhaps add a News.md with the trimmed list of changes.

rveltz commented 4 years ago

But then, how do you do a copyto! on RecursiveVec? You don't need this method in your entire package?

Jutho commented 2 years ago

Can I close this issue? Note that RecursiveVec and InnerProductVec do support copy!. I think that is the recommend method to copy the contents of a full vector into another one; copyto! seems (if I interpret this correctly) now to be a more specialised method for copying a part of a standard vector into another location, i.e. it seems like more of a low level type of memcopy method. But maybe I interpret Base functionality incorrectly.

rveltz commented 2 years ago

I dont remember but I think I opened this issue because my example despite fullfilling the requirements of your doc on vectors did not work. Let's close it for now