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

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

Closed
rveltz opened this issue Jun 20, 2020 · 9 comments
Closed

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

rveltz opened this issue Jun 20, 2020 · 9 comments

Comments

@rveltz
Copy link
Contributor

rveltz commented Jun 20, 2020

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
Copy link
Contributor Author

rveltz commented Jun 20, 2020

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
Copy link
Owner

Jutho commented Jun 20, 2020

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
Copy link
Owner

Jutho commented Jun 21, 2020

@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
Copy link
Contributor Author

rveltz commented Jun 21, 2020

It was me using copyto! on RecursiveVec...

No worries for the changes,

Bests

@Jutho
Copy link
Owner

Jutho commented Jun 21, 2020

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
Copy link
Contributor Author

rveltz commented Jun 21, 2020

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
Copy link
Contributor Author

rveltz commented Jul 16, 2020

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

@Jutho
Copy link
Owner

Jutho commented Jan 18, 2022

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
Copy link
Contributor Author

rveltz commented Jan 18, 2022

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

@Jutho Jutho closed this as completed Mar 16, 2022
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

No branches or pull requests

2 participants