Should we avoid Vec constructor for AbstractVector?

[ErickChacon]

This is related to JuliaEarth/GeoTables.jl#13, but in general it seems that is not a good idea to have non typestable contructor like the one defined for Vec over AbstracVector:

Meshes.jl/src/vectors.jl

Line 66 in cb556d3

Vec(coords::AbstractVector{T}) where {T} = Vec{length(coords),T}(coords)

.

For example, StaticArrays.jl avoids it (https://github.com/JuliaArrays/StaticArrays.jl/blob/ed92217f94e2d70847bcf2b7716f47a410d52d25/docs/src/pages/api.md?plain=1#L237-L239).

We have avoided adding SVector(v::AbstractVector) as a valid constructor to
help users avoid the type instability (and potential performance disaster, if
used without care) of this innocuous looking expression.

@juliohm Should we avoid using the constructor for AbstractVector or limit it for few dimensions?

[juliohm]

@ErickChacon thank you for sharing this information. I think it makes sense to follow the suggestion in StaticArrays.jl, mind submitting a PR?

[ErickChacon]

@juliohm, happy to submit a PR. If we remove the constructor for AbstractVector, the way to solve JuliaEarth/GeoTables.jl#13 would be using Point(coords...) like the following? It works, are you OK with that?

import ArchGDAL as AG
import GeoInterface as GI
using Meshes

mypoint = AG.createpoint(1.0, 5.0)

function topoint(geom)
    coords = GI.coordinates(geom)
    Meshes.Point(coords...)
end

topoint(mypoint)

[juliohm]

I understood that the idea was not to remove the constructor with AbstractVector, but to use the length function inside the function body with D=1,2,3, right? That doesn't solve the problem?

[ErickChacon]

Yes, limiting to 1d, 2d, 3d solves the problem. However, I thought it would be good idea to follow StaticArrays.jl approach and avoid the constructor over AbstractVector. For now, I will make the PR limiting the function to few dimensions.

[juliohm]

Oh I got it now. Can you remove the constructor with AbstractVector then to see if the test suite passes? I think we should follow StaticArrays.jl suggestion.

[ErickChacon]

I am removing the AbstractVector constructor, but I also will need to change some some tests. Is that ok?

[juliohm]

It sounds correct to me. Let's see what is the impact of removing this constructor in the PR and then we can figure out what is the best way forward. It should probably be fine to eliminate this constructor everywhere.

[ErickChacon]

Great, I will remove it everywhere. There are also several tests that fails in Intersections when removing this constructor, but it is mainly because the AbstractVector constructor is used in few lines.

[ErickChacon]

For collections we have this constructor

Meshes.jl/src/collections.jl

Line 56 in cb556d3

PointSet(coords::AbstractMatrix) = PointSet(Point.(eachcol(coords)))

that needs Vec for AbstractVector. Any suggestion?

[juliohm]

Should we convert the columns to tuples before sending to Point? Em sex., 28 de abr. de 2023 11:53, Dr. Erick A. Chacón Montalván < ***@***.***> escreveu:

…

For collections we have this constructor https://github.com/JuliaGeometry/Meshes.jl/blob/cb556d34900c64b6f40ac827360039c0fb3549f3/src/collections.jl#L56 that needs Vec for AbstractVector. Any suggestion? — Reply to this email directly, view it on GitHub <#398 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZQW3LHRGZXOM3RQDUTCKTXDPKXTANCNFSM6AAAAAAXOOQQRI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ErickChacon]

Maybe something like the following?

PointSet(coords::AbstractMatrix) = PointSet(Point.(Tuple.(eachcol(coords))))

[juliohm]

I would just do:

PointSet(coords::AbstractMatrix) = PointSet(Tuple.(eachcol(coords)))

given that there is another constructor for it already. Try to reduce to one of the previous constructors.

[ErickChacon]

The following operation does not work.

Meshes.jl/src/intersections/lines.jl

Line 66 in cb556d3

λ = A \ y

Where A is an SMatrix and y is a Vec. The reason for this is that StaticArrays.jl tries to convert them to the same type before making the operation. Previously, it worked because converted A to Vec and later make the a operation.

It can be solved by using

λ = A \ SVector(y.coords)

Is there a beteer approach?

[juliohm]

Maybe we should convert A and y in the very beginning of the algorithm to make them vanilla StaticVector and StaticMatrix? That way the entire code will work and we won't hit weird performance bugs.

[ErickChacon]

Yes, that would be better. I am almost done, so I will start the pull request and move the discussion there.