Fix #117 (#118)

* fix #117; better tests

* cleanup

* addres mfh's comments

---------

Co-authored-by: Christoph Ortner <[email protected]>

src/implementation/fast_system.jl

@@ -16,8 +16,9 @@ struct FastSystem{D, TCELL, L <: Unitful.Length, M <: Unitful.Mass, S} <: Abstra
 end
 
 # Constructor to fetch the types
-function FastSystem(box::NTuple{D, <: AbstractVector}, pbc::NTuple{D, Bool}, 
-                    positions, species, masses) where {D}
+function FastSystem(box::AUTOBOX, 
+                    pbc::AUTOPBC, 
+                    positions, species, masses)
     cϵll = PeriodicCell(; cell_vectors = box, periodicity = pbc)
     FastSystem(cϵll, positions, species, masses)
 end 
@@ -39,18 +40,20 @@ function FastSystem(system::AbstractSystem)
 end
 
 # Convenience constructor where we don't have to preconstruct all the static stuff...
-function FastSystem(particles, box, pbc)
-    D = length(box)
-    if !all(length.(box) .== D)
+function FastSystem(particles, box::AUTOBOX, pbc::AUTOPBC)
+    box1 = _auto_cell_vectors(box)
+    pbc1 = _auto_pbc(pbc, box1)
+    D = length(box1)
+    if !all(length.(box1) .== D)
         throw(ArgumentError("Box must have D vectors of length D=$D."))
     end
-    if length(pbc) != D
+    if length(pbc1) != D
         throw(ArgumentError("Boundary conditions should be of length D=$D."))
     end
     if !all(n_dimensions.(particles) .== D)
         throw(ArgumentError("Particles must have positions of length D=$D."))
     end
-    FastSystem(box, pbc, position.(particles), species.(particles), mass.(particles))
+    FastSystem(box1, pbc1, position.(particles), species.(particles), mass.(particles))
 end
 
 Base.length(sys::FastSystem)         = length(sys.position)

src/implementation/flexible_system.jl

@@ -49,20 +49,12 @@ Construct a flexible system, a versatile data structure for atomistic systems,
 which puts an emphasis on flexibility rather than speed.
 """
 function FlexibleSystem(
-    particles::AbstractVector{S},
-    box::NTuple{D, <: AbstractVector{L}},
-    periodicity::Union{Bool, NTuple{D, Bool}, AbstractVector{<: Bool}};
-    kwargs...
-) where {L<:Unitful.Length, S, D}
-    if periodicity isa Bool 
-        periodicity = ntuple(_ -> periodicity, D)
-    else 
-        periodicity = tuple(periodicity...)
-    end
-    if !all(length.(box) .== D)
-        throw(ArgumentError("Box must have D vectors of length D"))
-    end
-    cϵll = PeriodicCell(; cell_vectors = box, periodicity = periodicity)
+        particles::AbstractVector{S},
+        box::AUTOBOX{D},
+        pbc::AUTOPBC{D};
+        kwargs...
+    ) where {S, D}
+    cϵll = PeriodicCell(; cell_vectors = box, periodicity = pbc)
     FlexibleSystem{D, S, typeof(cϵll)}(particles, cϵll, Dict(kwargs...))
 end
 

src/implementation/utils.jl

@@ -19,7 +19,7 @@ julia> bounding_box = [[10.0, 0.0, 0.0], [0.0, 10.0, 0.0], [0.0, 0.0, 10.0]]u"Å
 julia> pbcs = (true, true, false)
 julia> hydrogen = atomic_system([:H => [0, 0, 1.]u"bohr",
                                  :H => [0, 0, 3.]u"bohr"],
-                                  bounding_box, pubcs)
+                                  bounding_box, pbcs)
 ```
 """
 atomic_system(atoms::AbstractVector{<:Atom}, box, bcs; kwargs...) = 
@@ -53,9 +53,9 @@ isolated_system(atoms::AbstractVector; kwargs...) =
 
 
 """
-    periodic_system(atoms::AbstractVector, bounding_box; fractional=false, kwargs...)
+    periodic_system(atoms::AbstractVector, box; fractional=false, kwargs...)
 
-Construct a [`FlexibleSystem`](@ref) with all boundaries of the `bounding_box` periodic
+Construct a [`FlexibleSystem`](@ref) with all boundaries of the `box` periodic
 (standard setup for modelling solid-state systems). If `fractional` is true, atom coordinates
 are given in fractional (and not in Cartesian) coordinates.
 Extra `kwargs` are stored as custom system properties.
@@ -71,24 +71,24 @@ julia> hydrogen = periodic_system([:H => [0, 0, 1.]u"bohr",
 
 Setup a silicon unit cell using fractional positions
 ```julia-repl
-julia> bounding_box = 10.26 / 2 * [[0, 0, 1], [1, 0, 1], [1, 1, 0]]u"bohr"
+julia> box = 10.26 / 2 * [[0, 0, 1], [1, 0, 1], [1, 1, 0]]u"bohr"
 julia> silicon = periodic_system([:Si =>  ones(3)/8,
                                   :Si => -ones(3)/8],
-                                 bounding_box, fractional=true)
+                                 box, fractional=true)
 ```
 """
 function periodic_system(atoms::AbstractVector,
-                         box::Union{Tuple, AbstractVector};
+                         box::AUTOBOX;
                          fractional=false, kwargs...)
-    pbcs = fill(true, length(box))
-    lattice = tuple(box...)
-    !fractional && return atomic_system(atoms, box, pbcs; kwargs...)
+    lattice = _auto_cell_vectors(box)
+    pbcs = fill(true, length(lattice))
+    !fractional && return atomic_system(atoms, lattice, pbcs; kwargs...)
 
     parse_fractional(atom::Atom) = atom
     function parse_fractional(atom::Pair)::Atom
         id, pos_fractional = atom
         Atom(id, sum(lattice .* pos_fractional))
     end
-    atomic_system(parse_fractional.(atoms), box, pbcs; kwargs...)
+    atomic_system(parse_fractional.(atoms), lattice, pbcs; kwargs...)
 end
 

src/utils/cells.jl

@@ -39,12 +39,12 @@ Implementation of a computational cell for particle systems
 """
 struct PeriodicCell{D, T}
    cell_vectors::NTuple{D, SVector{D, T}} 
-   pbc::NTuple{D, Bool}
+   periodicity::NTuple{D, Bool}
 end
 
 bounding_box(cell::PeriodicCell) = cell.cell_vectors 
 
-periodicity(cell::PeriodicCell) = cell.pbc
+periodicity(cell::PeriodicCell) = cell.periodicity
 
 n_dimensions(::PeriodicCell{D}) where {D} = D
 
@@ -62,7 +62,7 @@ PeriodicCell(cl::Union{AbstractSystem, PeriodicCell}) =
 
 function Base.show(io::IO, cϵll::PeriodicCell{D}) where {D} 
    u = unit(first(cϵll.cell_vectors[1][1]))
-   print(io, "PeriodicCell(", prod(p -> p ? "T" : "F", cϵll.pbc), ", ")  
+   print(io, "PeriodicCell(", prod(p -> p ? "T" : "F", periodicity(cϵll)), ", ")
    for d = 1:D 
       print(io, ustrip.(cϵll.cell_vectors[d]), u)
       if d < D; print(io, ", "); end 
@@ -75,6 +75,16 @@ end
 # ---------------------------------------------
 #     Utilities 
 
+# allowed input types that convert automatically to the 
+# intended format for cell vectors,  NTuple{D, SVector{D, T}}
+const AUTOBOX = Union{NTuple{D, <: AbstractVector}, 
+                      AbstractVector{<: AbstractVector}} where {D}
+
+# allowed input types that convert automatically to the 
+# intended format for pbc,  NTuple{D, Bool}
+const AUTOPBC = Union{Bool, 
+                      NTuple{D, Bool}, 
+                      AbstractVector{<: Bool}} where {D} 
 
 # different ways to construct cell vectors 
 
@@ -86,7 +96,7 @@ function _auto_cell_vectors(vecs::Tuple)
    return ntuple(i -> SVector{D}(vecs[i]), D)
 end
 
-_auto_cell_vectors(vecs::AbstractVector) = 
+_auto_cell_vectors(vecs::AbstractVector{<: AbstractVector}) = 
       _auto_cell_vectors(tuple(vecs...))
 
 # .... could consider allowing construction from a matrix but 
@@ -95,7 +105,7 @@ _auto_cell_vectors(vecs::AbstractVector) =
 
 # different ways to construct PBC 
 
-_auto_pbc1(bc::Bool)   = bc 
+_auto_pbc1(pbc::Bool)  = pbc 
 _auto_pbc1(::Nothing)  = false 
 
 _auto_pbc(bc::Tuple, cell_vectors = nothing) = 

test/test_docstrings.jl

@@ -0,0 +1,65 @@
+
+# this set of tests is intended to confirm that docstrings throughout the 
+# codebase actually run. Currently, this is just a rough draft that includes 
+# specific doc strings that have been reported to fail. There should be a more 
+# natural and automated way to test this. 
+
+using Unitful
+using UnitfulAtomic
+using AtomsBase
+using Test 
+
+## 
+# atomic_system docstring
+
+try 
+   bounding_box = [[10.0, 0.0, 0.0], [0.0, 10.0, 0.0], [0.0, 0.0, 10.0]]u"Å"
+   pbcs = (true, true, false)
+   hydrogen = atomic_system([:H => [0, 0, 1.]u"bohr",
+                                 :H => [0, 0, 3.]u"bohr"],
+                                  bounding_box, pbcs)
+   @test true
+catch 
+   @error("atomic_system docstring failed to run")
+   @test false 
+end
+
+
+##
+# isolated_system docstring
+
+try 
+   isolated_system([:H => [0, 0, 1.]u"bohr", :H => [0, 0, 3.]u"bohr"])
+   @test true
+catch
+   @error("isolated_system docstring failed to run")
+   @test false 
+end
+
+##
+# periodic_system docstring 1 
+
+try 
+   bounding_box = ([10.0, 0.0, 0.0]u"Å", [0.0, 10.0, 0.0]u"Å", [0.0, 0.0, 10.0]u"Å")
+   hydrogen = periodic_system([:H => [0, 0, 1.]u"bohr",
+                                      :H => [0, 0, 3.]u"bohr"],
+                                     bounding_box)
+   @test true 
+catch e 
+   @error("periodic_system docstring 1 failed to run")
+   @test false 
+end 
+
+##
+# periodic
+
+try 
+   box = 10.26 / 2 * [[0, 0, 1], [1, 0, 1], [1, 1, 0]]u"bohr"
+   silicon = periodic_system([:Si =>  ones(3)/8,
+                                     :Si => -ones(3)/8],
+                                    box, fractional=true)
+   @test true 
+catch e
+   @error("periodic_system docstring 2 failed to run")
+   @test false 
+end