JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
368 stars 53 forks source link

Segmentation fault calling `get_vector_orthogonal` #635

Closed olivierverdier closed 1 year ago

olivierverdier commented 1 year ago

The call is get_vector_orthogonal(Rotations(1), [1.;;], Float64[], ℝ). Admittedly, this is a corner case (I ended up there by mistake), but it would maybe still be nice to avoid the segfault for the user's sake...

mateuszbaran commented 1 year ago

Which version of Julia are you using? Julia 1.9.1 gives me [0.0;;], while 1.6.7 throws an assertion error.

olivierverdier commented 1 year ago

Ah, interesting. Not easy to figure out what is wrong then... Here is my full version info:

Julia Version 1.9.0
Commit 8e630552924 (2023-05-07 11:25 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 8 × Apple M1
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, apple-m1)
  Threads: 1 on 4 virtual cores
Environment:
  JULIA_EDITOR = emacs
  JULIA_INPUT_COLOR = bold
  JULIA_ANSWER_COLOR = bold

Should I just upgrade to Julia v1.9.1? Or do you have any idea as to how I could investigate further?

mateuszbaran commented 1 year ago

I think you should upgrade to 1.9.1, it might be a weird Julia bug. If it still segfaults then I will try to reproduce it using our CI.

olivierverdier commented 1 year ago

Some more info about the segfault: the bug occurs because the call above passes through this line: X[1, 2] = -Xⁱ[3], the problem is that Xⁱ is Float64[] and the lines lies within an @inbounds section.

sethaxen commented 1 year ago

There are a few ways we can handle this. Probably the cleanest would be to throw an error when one tries to construct a GeneralUnitaryMatrices{1,ℝ}, since that has a manifold dimension of 0. Then this issue could never be hit.

mateuszbaran commented 1 year ago

I see, that indeed should be fixed. I'm not sure if we really want to disallow Rotations(1)? That's a valid manifold after all and we could just add a method

 function get_vector_orthogonal!(
    M::GeneralUnitaryMatrices{1,ℝ},
    X,
    p,
    Xⁱ,
    ::RealNumbers,
)
    return X .= 0
end
kellertuer commented 1 year ago

But Rotations(1) would only consists of the 1x1 matrix containing a 1? Sure disallowing it might be a bit too strong, so then I would probably prefer either your fix setting X to zero or return even an empty vector [] since 0 might indicate a 1D tangent space while it is 0D?

mateuszbaran commented 1 year ago

Yes, Rotations(1) only has one point, with zero-dimensional tangent space consisting of just the zero vector.

since 0 might indicate a 1D tangent space while it is 0D?

We don't consider it a problem for other manifolds. A tangent vector to Sphere(2) is a vector of length 3 despite the fact that the tangent space is two-dimensional. It could work similarly here. Of course, the vector of coefficients Xⁱ needs to be empty.

sethaxen commented 1 year ago

Agreed, but we also then need to check that our other methods also do not assume the point or tangent is nonempty.

mateuszbaran commented 1 year ago

I will check other methods and make a PR then.