Named access with .{x,y,z,w} to elements of SVector and MVector #980

c42f · 2021-12-16T03:39:43Z

Having access with v.x etc makes the concrete SVector interface much
more usable for geometry; indeed, it will "just work like you expect"
without the need to resort to a custom FieldVector type.

This makes the concrete SVector/MVector interface fatter, but OTOH I
don't think it can do any harm to people who don't want to use it.

If we do this, it should be made to work at least for SVector{1}
SVector{2} and SVector{3}, but I saw no reason to disallow the use of
these in higher dimensions and overcomplicate the implementation.

Inspired by discussion at

https://discourse.julialang.org/t/ray-tracing-in-a-week-end-julia-vs-simd-optimized-c/72958

I'll add tests and some docs if we decide this is a good idea!

andyferris

Yes, I can see how this could be convenient for using these as points, and as you say there isn’t any real downside.

andyferris · 2021-12-16T05:53:49Z

src/MVector.jl

+
+# Named field access for the first three elements with the conventional
+# field names
+function Base.getproperty(v::Union{MVector}, name::Symbol)


Unnecessary Union

andyferris · 2021-12-16T05:54:26Z

src/SVector.jl

+
+# Named field access for the first three elements with the conventional
+# field names
+function Base.getproperty(v::Union{SVector}, name::Symbol)


Unnecessary Union

Heh oops, thanks!

The reason this was there was that I had this defined as Union{SVector,MVector} because the implementations are the same.

But then I didn't know which file to put these in so I duplicated them instead as they're a concrete interface. Kind of a stupid reason to have duplicate code though I guess...

c42f · 2021-12-16T07:16:56Z

Perhaps I should also add setproperty! for MVector to go with this.

src/MVector.jl

c42f · 2021-12-16T11:28:11Z

src/SVector.jl

@@ -36,10 +36,6 @@ const SVector{S, T} = SArray{Tuple{S}, T, 1, S}
 ## SVector methods ##
 #####################

-@propagate_inbounds function getindex(v::SVector, i::Int)
-    v.data[i]
-end


Haha, this redundant method has been in here for years. I removed it in favour of the linear getindex from SArray

c42f · 2021-12-16T11:42:07Z

Absent objections, I'll probably merge this some time tomorrow or so.

It's a neat little feature and should be harmless to anyone who hasn't been involved in type piracy :-)

ffreyer · 2021-12-16T13:00:49Z

Perhaps this should also include p.w = p[4] to complete the set that's used in glsl?

cdsousa · 2021-12-16T13:31:40Z

Perhaps this should also include p.w = p[4] to complete the set that's used in glsl?

It is also used by the Eigen C++ library.

c42f · 2021-12-16T20:30:00Z

I agree w is an obvious option for the forth conventional dimension name. Perhaps it's the best of the available options. If you're a physicist, the most obvious fourth dimension name might be t for time. (No it's ok, we're not doing that 😆)

I initially decided against w because it's not quite so standard — it's more a graphics (homogeneous coordinates) convention than a universal geometry thing...

fredrikekre · 2021-12-16T21:34:04Z

Would it make sense to limit this to length-3 vectors (.x for 1D, .x, .y for 2D and .x, .y, .z for 3D)?

claforte · 2021-12-16T21:52:27Z

Maybe throw @inbounds in there? I think that made a significant speed-up in my toy raytracing implementation, and otherwise people might not think to add it up, i.e. might not realize that .x/.y translates into array look-up...

c42f · 2021-12-17T00:48:54Z

Would it make sense to limit this to length-3 vectors (.x for 1D, .x, .y for 2D and .x, .y, .z for 3D)?

I don't know! The question is, could it lead to unexpected bugs in other cases?

You know what I think I agree: let's be strict here and relax the rules later if we think it makes sense. It's much easier to add API than to remove it!

Maybe throw @inbounds in there?

Adding @inbounds here shouldn't help because any bounds check should be elided given constant propagation of the constant index into the data tuple. Though on that note, it would be better (easier on the compiler) to just implement this in terms of getfield(v,:data) rather than getindex — ~~that avoids getproperty being recursive. I'll do that!~~ ( turns out I already did this... duh!)

c42f · 2021-12-17T02:09:29Z

Ok thanks for the feedback everyone. I've now updated this as follows:

Restricted this to work with vectors up to dimension 4 only. We can always allow for higher dimensions later, but I doubt that'll be necessary.
Added the .w component. GLSL is pretty great I agree.
Added @inbounds to setindex usage so the compiler can elide this as early as possible. It seems nice not to rely on constant prop there.
Updated docs and tests accordingly

c42f · 2021-12-17T02:11:27Z

Side note... gosh our docs are pretty awful 😅

Not something for this PR though.

docs/src/pages/quickstart.md

Having access with v.x etc makes the *concrete* SVector interface much more usable for geometry; indeed, it will "just work like you expect" without the need to resort to a custom FieldVector type. This makes the concrete SVector/MVector interface fatter, but I don't think it can do any harm to people who don't want to use it.

andyferris approved these changes Dec 16, 2021

View reviewed changes

mateuszbaran reviewed Dec 16, 2021

View reviewed changes

src/MVector.jl Outdated Show resolved Hide resolved

c42f commented Dec 16, 2021

View reviewed changes

mateuszbaran approved these changes Dec 16, 2021

View reviewed changes

SamuelBadr reviewed Dec 17, 2021

View reviewed changes

docs/src/pages/quickstart.md Outdated Show resolved Hide resolved

c42f force-pushed the cjf/svector-getproperty branch from 4ece173 to 8ef637f Compare December 17, 2021 06:39

c42f merged commit c1620f0 into master Dec 18, 2021

c42f deleted the cjf/svector-getproperty branch December 18, 2021 02:09

c42f changed the title ~~Named access with .{x,y,z} to elements of SVector and MVector~~ Named access with .{x,y,z,w} to elements of SVector and MVector Dec 24, 2021

cdsousa mentioned this pull request Jan 1, 2022

Replicate named access with .{x,y,z,w} from StaticArrays https://github.com/JuliaArrays/StaticArrays.jl/pull/980 JuliaGeometry/GeometryBasics.jl#157

Open

stevengj mentioned this pull request Jan 3, 2022

note named access for SVector in FieldVector docs #983

Merged

hyrodium mentioned this pull request Jan 6, 2022

Change the order of QuatRotation from (w,x,y,z) to (x,y,z,w) JuliaGeometry/Rotations.jl#210

Open

visr mentioned this pull request Jan 9, 2022

Rename package to Proj.jl JuliaGeo/Proj.jl#57

Merged

jipolanco mentioned this pull request Apr 29, 2022

Illustrate broadcasting over wavenumber grids (or wavenumber PencilArray) in gradient.jl ? jipolanco/PencilFFTs.jl#42

Closed

serenity4 mentioned this pull request Nov 24, 2023

Extend getproperty and setproperty! with swizzling for vectors #1221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Named access with .{x,y,z,w} to elements of SVector and MVector #980

Named access with .{x,y,z,w} to elements of SVector and MVector #980

c42f commented Dec 16, 2021 •

edited

Loading

andyferris left a comment

andyferris Dec 16, 2021

andyferris Dec 16, 2021

c42f Dec 16, 2021

c42f commented Dec 16, 2021

c42f Dec 16, 2021

c42f commented Dec 16, 2021

ffreyer commented Dec 16, 2021 •

edited

Loading

cdsousa commented Dec 16, 2021

c42f commented Dec 16, 2021

fredrikekre commented Dec 16, 2021

claforte commented Dec 16, 2021

c42f commented Dec 17, 2021 •

edited

Loading

c42f commented Dec 17, 2021

c42f commented Dec 17, 2021

Named access with .{x,y,z,w} to elements of SVector and MVector #980

Named access with .{x,y,z,w} to elements of SVector and MVector #980

Conversation

c42f commented Dec 16, 2021 • edited Loading

andyferris left a comment

Choose a reason for hiding this comment

andyferris Dec 16, 2021

Choose a reason for hiding this comment

andyferris Dec 16, 2021

Choose a reason for hiding this comment

c42f Dec 16, 2021

Choose a reason for hiding this comment

c42f commented Dec 16, 2021

c42f Dec 16, 2021

Choose a reason for hiding this comment

c42f commented Dec 16, 2021

ffreyer commented Dec 16, 2021 • edited Loading

cdsousa commented Dec 16, 2021

c42f commented Dec 16, 2021

fredrikekre commented Dec 16, 2021

claforte commented Dec 16, 2021

c42f commented Dec 17, 2021 • edited Loading

c42f commented Dec 17, 2021

c42f commented Dec 17, 2021

c42f commented Dec 16, 2021 •

edited

Loading

ffreyer commented Dec 16, 2021 •

edited

Loading

c42f commented Dec 17, 2021 •

edited

Loading