From 86a6d932273dd7ff99ce06059cae9fc765f698a9 Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Tue, 21 Mar 2023 14:57:28 +0100 Subject: [PATCH 1/5] Make AtomView rely on struct-of-array format --- docs/src/apireference.md | 1 + src/atomview.jl | 44 +++++++++++++++++++++++++++++++++------- src/fast_system.jl | 14 +++---------- test/fast_system.jl | 2 ++ test/printing.jl | 4 ++-- 5 files changed, 45 insertions(+), 20 deletions(-) diff --git a/docs/src/apireference.md b/docs/src/apireference.md index d93ae1d..b1cf479 100644 --- a/docs/src/apireference.md +++ b/docs/src/apireference.md @@ -38,6 +38,7 @@ element ```@docs Atom +AtomView FlexibleSystem AbstractSystem atomic_system diff --git a/src/atomview.jl b/src/atomview.jl index 51adeb6..e377c2a 100644 --- a/src/atomview.jl +++ b/src/atomview.jl @@ -2,15 +2,45 @@ # A simple view datastructure for atoms of struct of array systems # export AtomView -struct AtomView{FS <: AbstractSystem} - system::FS + +""" + AtomView{S<:AbstractSystem} + +Species type for atoms of systems implemented as struct-of-arrays. +Can be queried with the same API than for other species, like [`Atom`](@ref). + +See [FastSystem](@ref Struct-of-Arrays-/-FastSystem) for an example of system +using `AtomView` as its species type. + +## Example +```jldoctest; setup=:(using AtomsBase, Unitful; atoms = Atom[:C => [0.25, 0.25, 0.25]u"Å", :C => [0.75, 0.75, 0.75]u"Å"]; box = [[1, 0, 0], [0, 1, 0], [0, 0, 1]]u"Å"; boundary_conditions = [Periodic(), Periodic(), DirichletZero()]) +julia> system = FastSystem(atoms, box, boundary_conditions); + +julia> atom = system[2] +AtomView(C, atomic_number = 6, atomic_mass = 12.011 u): + position : [0.75,0.75,0.75]u"Å" + +julia> atom isa AtomView{typeof(system)} +true + +julia> atomic_symbol(atom) +:C +``` +""" +struct AtomView{S<:AbstractSystem} + system::S index::Int end -velocity(v::AtomView) = velocity(v.system, v.index) -position(v::AtomView) = position(v.system, v.index) -atomic_mass(v::AtomView) = atomic_mass(v.system, v.index) -atomic_symbol(v::AtomView) = atomic_symbol(v.system, v.index) -atomic_number(v::AtomView) = atomic_number(v.system, v.index) + +function velocity(v::AtomView) + vel = velocity(v.system) + ismissing(vel) && return missing + return vel[v.index] +end +position(v::AtomView) = position(v.system)[v.index] +atomic_mass(v::AtomView) = atomic_mass(v.system)[v.index] +atomic_symbol(v::AtomView) = atomic_symbol(v.system)[v.index] +atomic_number(v::AtomView) = atomic_number(v.system)[v.index] n_dimensions(v::AtomView) = n_dimensions(v.system) element(atom::AtomView) = element(atomic_number(atom)) diff --git a/src/fast_system.jl b/src/fast_system.jl index 9e4d107..6284d0a 100644 --- a/src/fast_system.jl +++ b/src/fast_system.jl @@ -55,19 +55,11 @@ atomic_number(s::FastSystem) = s.atomic_number atomic_mass(s::FastSystem) = s.atomic_mass velocity(::FastSystem) = missing -position(s::FastSystem, i) = s.position[i] -atomic_symbol(s::FastSystem, i) = s.atomic_symbol[i] -atomic_number(s::FastSystem, i) = s.atomic_number[i] -atomic_mass(s::FastSystem, i) = s.atomic_mass[i] -velocity(::FastSystem, i) = missing - # System property access function Base.getindex(system::FastSystem, x::Symbol) - if x in (:bounding_box, :boundary_conditions) - getfield(system, x) - else - throw(KeyError("Key $x not found")) - end + x === :bounding_box && return bounding_box(system) + x === :boundary_conditions && return boundary_conditions(system) + throw(KeyError(x)) end Base.haskey(::FastSystem, x::Symbol) = x in (:bounding_box, :boundary_conditions) Base.keys(::FastSystem) = (:bounding_box, :boundary_conditions) diff --git a/test/fast_system.jl b/test/fast_system.jl index 9e1f4a1..e1b885c 100644 --- a/test/fast_system.jl +++ b/test/fast_system.jl @@ -20,6 +20,8 @@ using PeriodicTable @test !isinfinite(system) @test element(system[1]) == element(:C) @test keys(system) == (:bounding_box, :boundary_conditions) + @test haskey(system, :boundary_conditions) + @test system[:boundary_conditions][1] == Periodic() @test atomkeys(system) == (:position, :atomic_symbol, :atomic_number, :atomic_mass) @test keys(system[1]) == (:position, :atomic_symbol, :atomic_number, :atomic_mass) @test hasatomkey(system, :atomic_symbol) diff --git a/test/printing.jl b/test/printing.jl index 25b83b2..3212a40 100644 --- a/test/printing.jl +++ b/test/printing.jl @@ -13,12 +13,12 @@ using Test flexible_system = periodic_system(atoms, box; fractional=true, data=-12) @test repr(flexible_system) == """ - FlexibleSystem(CSi, periodic = TTT, bounding_box = [[10.0, 0.0, 0.0], [0.0, 5.0, 0.0], [0.0, 0.0, 7.0]]u"Å")""" + FlexibleSystem(CSi, periodic = TTT, bounding_box = [[10.0, 0.0, 0.0], [0.0, 5.0, 0.0], [0.0, 0.0, 7.0]]u"Å")""" show(stdout, MIME("text/plain"), flexible_system) fast_system = FastSystem(flexible_system) @test repr(fast_system) == """ - FastSystem(CSi, periodic = TTT, bounding_box = [[10.0, 0.0, 0.0], [0.0, 5.0, 0.0], [0.0, 0.0, 7.0]]u"Å")""" + FastSystem(CSi, periodic = TTT, bounding_box = [[10.0, 0.0, 0.0], [0.0, 5.0, 0.0], [0.0, 0.0, 7.0]]u"Å")""" show(stdout, MIME("text/plain"), fast_system) show(stdout, MIME("text/plain"), fast_system[1]) end From e229b18334fa0d5543e84647bd7b85e66f2f0a5d Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Tue, 21 Mar 2023 18:16:12 +0100 Subject: [PATCH 2/5] Simple if branch instead of && short-circuit --- src/fast_system.jl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/fast_system.jl b/src/fast_system.jl index 6284d0a..2770ceb 100644 --- a/src/fast_system.jl +++ b/src/fast_system.jl @@ -57,8 +57,12 @@ velocity(::FastSystem) = missing # System property access function Base.getindex(system::FastSystem, x::Symbol) - x === :bounding_box && return bounding_box(system) - x === :boundary_conditions && return boundary_conditions(system) + if x === :bounding_box + return bounding_box(system) + end + if x === :boundary_conditions + return boundary_conditions(system) + end throw(KeyError(x)) end Base.haskey(::FastSystem, x::Symbol) = x in (:bounding_box, :boundary_conditions) From 0bbb428af8ac90803f4def98d382d1d297be96ba Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Wed, 22 Mar 2023 10:22:01 +0100 Subject: [PATCH 3/5] Unify getindex for fast and flexible systems --- src/fast_system.jl | 10 +++++----- src/flexible_system.jl | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/fast_system.jl b/src/fast_system.jl index 2770ceb..c25d9b1 100644 --- a/src/fast_system.jl +++ b/src/fast_system.jl @@ -58,12 +58,12 @@ velocity(::FastSystem) = missing # System property access function Base.getindex(system::FastSystem, x::Symbol) if x === :bounding_box - return bounding_box(system) + bounding_box(system) + elseif x === :boundary_conditions + boundary_conditions(system) + else + throw(KeyError(x)) end - if x === :boundary_conditions - return boundary_conditions(system) - end - throw(KeyError(x)) end Base.haskey(::FastSystem, x::Symbol) = x in (:bounding_box, :boundary_conditions) Base.keys(::FastSystem) = (:bounding_box, :boundary_conditions) diff --git a/src/flexible_system.jl b/src/flexible_system.jl index 4f6112f..38cf6ed 100644 --- a/src/flexible_system.jl +++ b/src/flexible_system.jl @@ -13,8 +13,10 @@ end # System property access function Base.getindex(system::FlexibleSystem, x::Symbol) - if x in (:bounding_box, :boundary_conditions) - getfield(system, x) + if x === :bounding_box + bounding_box(system) + elseif x === :boundary_conditions + boundary_conditions(system) else getindex(system.data, x) end From bd269102ed48ae40037a88b656657eea0427b1fb Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Wed, 22 Mar 2023 10:39:41 +0100 Subject: [PATCH 4/5] Test type stability --- test/fast_system.jl | 7 +++++++ test/interface.jl | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/test/fast_system.jl b/test/fast_system.jl index e1b885c..81440a7 100644 --- a/test/fast_system.jl +++ b/test/fast_system.jl @@ -2,6 +2,7 @@ using AtomsBase using Test using Unitful using PeriodicTable +using StaticArrays @testset "Fast system" begin box = [[1, 0, 0], [0, 1, 0], [0, 0, 1]]u"m" @@ -41,6 +42,12 @@ using PeriodicTable :atomic_mass => atomic_mass(atoms[1]), ] + # check type stability + get_b_vector(syst) = bounding_box(syst)[2] + @test @inferred(get_b_vector(system)) === SVector{3}([0.0, 1.0, 0.0]u"m") + @test @inferred(position(system, 1)) === SVector{3}([0.25, 0.25, 0.25]u"m") + @test ismissing(@inferred(velocity(system, 2))) + # Test AtomView for method in (position, atomic_mass, atomic_symbol, atomic_number) @test method(system[1]) == method(system, 1) diff --git a/test/interface.jl b/test/interface.jl index 5554d54..43a33f5 100644 --- a/test/interface.jl +++ b/test/interface.jl @@ -56,5 +56,9 @@ using PeriodicTable @test ismissing(velocity(fast)) @test all(position(fast) .== position(flexible)) @test all(atomic_symbol(fast) .== atomic_symbol(flexible)) + + # type stability + get_z_periodicity(syst) = syst[:boundary_conditions][3] + @test @inferred(BoundaryCondition, get_z_periodicity(flexible)) === DirichletZero() end end From 40448744a5213250069f4db5b888c72868080013 Mon Sep 17 00:00:00 2001 From: Lionel Zoubritzky Date: Wed, 22 Mar 2023 15:37:51 +0100 Subject: [PATCH 5/5] Relax === tests to == --- test/fast_system.jl | 4 ++-- test/interface.jl | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/fast_system.jl b/test/fast_system.jl index 81440a7..419b923 100644 --- a/test/fast_system.jl +++ b/test/fast_system.jl @@ -44,8 +44,8 @@ using StaticArrays # check type stability get_b_vector(syst) = bounding_box(syst)[2] - @test @inferred(get_b_vector(system)) === SVector{3}([0.0, 1.0, 0.0]u"m") - @test @inferred(position(system, 1)) === SVector{3}([0.25, 0.25, 0.25]u"m") + @test @inferred(get_b_vector(system)) == SVector{3}([0.0, 1.0, 0.0]u"m") + @test @inferred(position(system, 1)) == SVector{3}([0.25, 0.25, 0.25]u"m") @test ismissing(@inferred(velocity(system, 2))) # Test AtomView diff --git a/test/interface.jl b/test/interface.jl index 43a33f5..085971e 100644 --- a/test/interface.jl +++ b/test/interface.jl @@ -59,6 +59,6 @@ using PeriodicTable # type stability get_z_periodicity(syst) = syst[:boundary_conditions][3] - @test @inferred(BoundaryCondition, get_z_periodicity(flexible)) === DirichletZero() + @test @inferred(BoundaryCondition, get_z_periodicity(flexible)) == DirichletZero() end end