chakravala / Grassmann.jl

⟨Grassmann-Clifford-Hodge⟩ multilinear differential geometric algebra
https://grassmann.crucialflow.com
GNU Affero General Public License v3.0
470 stars 38 forks source link

Grassmann should not define `ndims` for Julia Vectors #67

Closed eschnett closed 4 years ago

eschnett commented 4 years ago

I encountered this problem:

julia> using Grassmann

julia> D = 2
2

julia> S = Signature(D)
⟨++⟩

julia> V = SubManifold(S)
⟨++⟩

julia> x = Chain{V,1}(1,2)
1v₁ + 2v₂

julia> xs = [x]
1-element Array{Chain{⟨++⟩,1,Int64,2},1}:
 1v₁ + 2v₂

julia> @show xs
ERROR: _show_nonempty(::IO, ::AbstractVector, ::String) is not implemented
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] _show_nonempty(::IOContext{Base.GenericIOBuffer{Array{UInt8,1}}}, ::Array{Chain{⟨++⟩,1,Int64,2},1}, ::String) at ./arrayshow.jl:420
 [3] show(::Base.GenericIOBuffer{Array{UInt8,1}}, ::Array{Chain{⟨++⟩,1,Int64,2},1}) at ./arrayshow.jl:437
 [4] sprint(::Function, ::Array{Chain{⟨++⟩,1,Int64,2},1}; context::Nothing, sizehint::Int64) at ./strings/io.jl:105
 [5] #repr#353 at ./strings/io.jl:227 [inlined]
 [6] repr(::Array{Chain{⟨++⟩,1,Int64,2},1}) at ./strings/io.jl:227
 [7] top-level scope at show.jl:634

The problem seems to be that Grassmann defines (see multivectors.jl, line 139) the method

@pure Base.ndims(::Vector{Chain{V,G,T,X}} where {G,T,X}) where V = ndims(V)

which confuses the base library when showing vectors.

I think it's wrong to overload ndims for any Vector{T}. All Vector{T} are one-dimensional arrays. The corresponding definition for Base.parent probably also shouldn't be there.

This breaks my code since using @show is important for debugging for me.

chakravala commented 4 years ago

That method definition is essential for some implementations I'm working on.

Deleting this method is not currently an option.

Using display instead of @show seems to work for me.

eschnett commented 4 years ago

I wonder in what context you want to treat a vector of chains as a chain. It might be that you are really calling ndims(eltype(x)), or maybe the(map(ndims, x)). Or are you really dealing with Chain{V,G,Vector{T}}, but are using an inside-out internal representation?

The Julia way to handle the latter case would be to introduce a new type

@computed struct VChain{V,G,T}
    vchain::Vector{fulltype(VChain{V,G,T})}
end

That's a bit inconvenient at places, since one has to define a certain number of wrapper functions for VChain, but it avoids this kind of confusion.

(the(xs) is a function that accepts a non-empty collection of elements, expects all elements to be identical, and returns that element.)

chakravala commented 4 years ago

It's called a ChainBundle, it already has a new type, but this new type is only used if caching is needed, if caching is not needed then it's just simply a Vector{<:Chain} elements.

It's necessary for me to compute ndims of the elements of such a Vector. However, in a future release I might consider using a call like ndims(Manifold(::Vector{<:Chain{V}})) instead.

Deleting that method would be a breaking change for Adapode at the moment, but I'll look into changing it for a future release.

chakravala commented 4 years ago

This issue has been fixed now, by the way.