Move duplicated AD helpers into main package#48
Conversation
| function project_add!(C, A, α) | ||
| TC = Base.promote_op(+, scalartype(A), scalartype(α)) | ||
| return if !(TC <: Real) && scalartype(C) <: Real | ||
| add!(C, real(add!(zerovector(C, TC), A, α))) |
There was a problem hiding this comment.
The problem with this definition is of course that it is much more limited than the types that VI supports, because of the real call. Effectively, this will only support AbstractArray{<:Number}, not nested arrays like add! does.
I don't think there is a quick solution for that. One way might have to have a a project_add! implementation that is similar to add!, but that forces to also have project_add!! with implementations for tuples etc, as that is how add! is implemented.
An alternative, which might be useful in other places as well, is that VectorInterface adds a new method to its interface, namely project_real, that supports nested types. I certainly have contemplated about this in other places as well where I thought this could be useful, but then always found another way around this.
There was a problem hiding this comment.
I suppose one option would be to require such types to implement their own project_add!, which they can at least do more easily if it's actually a "first class citizen" package method?
There was a problem hiding this comment.
For other types, yes, but I mean, the current implementation already fails for something like A=[(1., 2.), (3., 4.)] which is currently supported by VectorInterface.add!.
There was a problem hiding this comment.
Then indeed maybe project_real is a good option, if nothing else so that examples like that don't blow up beneath people unexpectedly
There was a problem hiding this comment.
I do think this package might be a good place for project_real, since it does come up quite often, and I honestly can't really think of a better place to put it.
I do agree that we want to be careful with this package, since it is supposed to be very lightweight and the fact that we can tell people to overload very few operations is attractive, so if possible I would like a very generic fallback, and decent implementations for the Base datatypes?
It does look like it is very similar to the implementation of scale(y, x, alpha), with the addition of a real call on the actual Number types, so maybe this is actually not that hard to implement
There was a problem hiding this comment.
I do agree that we want to be careful with this package, since it is supposed to be very lightweight and the fact that we can tell people to overload very few operations is attractive
I wonder almost if it would make sense to create a new package, VectorInterfaceAD, which things resting on top of this could use (or not) without "polluting" the main package for those who don't give a damn about AD. But that also introduces its own problems of course...
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
3 participants
Surfaced elsewhere but it would be good to start consolidating all these