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

Adding different FixedVectors results in stack overflow #101

Closed
andyferris opened this issue Apr 28, 2016 · 6 comments
Closed

Adding different FixedVectors results in stack overflow #101

andyferris opened this issue Apr 28, 2016 · 6 comments

Comments

@andyferris
Copy link
Contributor

andyferris commented Apr 28, 2016

If you try add two FixedSizeArrays of different base types, then you may get stack overflows with common operations.

For example:

julia> Point(1,2) + Vec(1,2)
ERROR: StackOverflowError:
 in + at /home/aferris/.julia/v0.4/FixedSizeArrays/src/ops.jl:64 (repeats 513 times)

The problem comes back to this in ops.jl:

@inline $op{T, T2, NDIM, SIZE}(x::FixedArray{T, NDIM, SIZE}, y::FixedArray{T2, NDIM, SIZE}) = $op(promote(x, y)...)

which ends up calling itself, unless the base types are the same. There are two problems:

  1. Can we decide automatically what type to return? If not, maybe this should be an error...
  2. Once we have decided (I know the choice in my case), can we even in-principal do this? I suppose there is sufficient magic between promote and generated functions/function generators constructor_expr, etc? I will dig around.
@andyferris
Copy link
Contributor Author

andyferris commented Apr 28, 2016

OK I figured out how to do my use case:

function translate{N}(x::FixedVector{N}, dx::FixedVector{N})
    (x_promoted, dx_promoted) = promote(x, dx) # Force same data type
    x_promoted + (typeof(x_promoted))(dx_promoted) # Force same base type
end

This returns the appropriate container using x's base type. And @code_native says it's as fast as adding two Vecs, so there is no wastage.

Do we care about the stack overflow?

@c42f
Copy link
Collaborator

c42f commented Apr 28, 2016

The stack overflow definitely seems bad - should probably be either a no method error, or be promoted to Point. The latter depends on what Point is actually meant to be: is it an element of a vector space or an affine space? IMO the latter makes most conceptual sense and the following arithmetic rules are required:

Point ± Vec -> Point
Point - Point -> Vec
Vec - Point -> error
Point + Point -> error

TBH, the discussion of what Point actually represents probably deserves its own issue.

@andyferris
Copy link
Contributor Author

Seems to me like we need a system of abstractions:

FixedSizeVector <: FixedSizeAffine <: FixedSizeStorage

The storage knows how to index, concatenated, etc. The Affine space defines affine combinations. The vector space does all of the things in Vector/Vec, plus defines the operations pointed out by @c42f.

The problem is, base Julia needs exactly the same thing! Would people welcome such levels of abstraction? I can see the utility in geometric settings, and having lighter-weight storage containers makes sense to me also.

But, everything is wrapped up in Vector and Vec, and apart from safety issues, it all works...

@c42f
Copy link
Collaborator

c42f commented Apr 28, 2016

The difficult design issue is that extra abstraction always comes with a mental cost to understand each abstraction. On the other hand, you can stick a bunch of related abstractions together into a single type, but this can get conceptually messy if you go too far. To me, FixedArray should have exactly the same semantics as Array where possible, as Base users already have a good mental model of what to expect.

For better or worse Array is a bit of a bag of all tricks - depending on dimensionality it can be a vector, a linear operator, a multidimensional rectangular container, an efficient extensible list/stack (in 1D), etc.

@c42f
Copy link
Collaborator

c42f commented May 23, 2016

Fixed via #105

@c42f c42f closed this as completed May 23, 2016
@c42f
Copy link
Collaborator

c42f commented May 23, 2016

Note that the type is currently taken from the left hand side for vec + point, but this is a separate issue from the stack overflow :)

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