From 6b94f5708d91e17f9e0629044f730e7582c65738 Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Mon, 2 Sep 2024 11:34:57 -0700 Subject: [PATCH 1/6] draft - AB0.4 update --- Project.toml | 4 +-- src/atoms.jl | 79 ++++++++++++++++++++++++++-------------------------- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/Project.toml b/Project.toml index b7ced75..626a4f5 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "ExtXYZ" uuid = "352459e4-ddd7-4360-8937-99dcb397b478" authors = ["James Kermode and contributors"] -version = "0.1.15-DEV" +version = "0.2-DEV" [deps] AtomsBase = "a963bdd2-2df7-4f54-a1ee-49d51e6be12a" @@ -12,7 +12,7 @@ UnitfulAtomic = "a7773ee8-282e-5fa2-be4e-bd808c38a91a" extxyz_jll = "6ecdc6fc-93a8-5528-aee3-ac7ae1c60be7" [compat] -AtomsBase = "0.3" +AtomsBase = "0.3, 0.4" PeriodicTable = "1" StaticArrays = "1.5" Unitful = "1" diff --git a/src/atoms.jl b/src/atoms.jl index 31816ba..dcd6279 100644 --- a/src/atoms.jl +++ b/src/atoms.jl @@ -24,30 +24,30 @@ end function Atoms(system::AbstractSystem{D}) n_atoms = length(system) atomic_symbols = [Symbol(element(atomic_number(at)).symbol) for at in system] - if atomic_symbols != atomic_symbol(system) + if atomic_symbols != atomic_symbol(system, :) @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") end atom_data = Dict{Symbol,Any}( :atomic_symbol => atomic_symbols, :atomic_number => atomic_number(system), - :atomic_mass => atomic_mass(system) + :mass => mass(system) ) atom_data[:position] = map(1:n_atoms) do at pos = zeros(3)u"Å" pos[1:D] = position(system, at) - pos + SVector{D, eltype(pos)}(pos) # AtomsBase 0.4 requires SVector end atom_data[:velocity] = map(1:n_atoms) do at vel = zeros(3) * uVelocity if !ismissing(velocity(system)) && !ismissing(velocity(system, at)) vel[1:D] = velocity(system, at) end - vel + SVector{D, eltype(pos)}(vel) # AtomsBase 0.4 requires SVector end for k in atomkeys(system) - if k in (:atomic_symbol, :atomic_number, :atomic_mass, :velocity, :position) + if k in (:atomic_symbol, :atomic_number, :mass, :velocity, :position) continue # Already done end atoms_base_keys = (:charge, :covalent_radius, :vdw_radius, @@ -65,20 +65,14 @@ function Atoms(system::AbstractSystem{D}) end end - box = map(1:3) do i - v = zeros(3)u"Å" - i ≤ D && (v[1:D] = bounding_box(system)[i]) - v - end system_data = Dict{Symbol,Any}( - :bounding_box => box, - :boundary_conditions => boundary_conditions(system) + :bounding_box => bounding_box(system) # NTuple{D, SVector}, + :periodicity => periodicity(system) # NTuple{D, Bool} ) # Extract extra system properties - system_data = Dict{Symbol,Any}() for (k, v) in pairs(system) - atoms_base_keys = (:charge, :multiplicity, :boundary_conditions, :bounding_box) + atoms_base_keys = (:charge, :multiplicity, :periodicity, :bounding_box) if k in atoms_base_keys || v isa ExtxyzType || v isa AbstractArray{<: ExtxyzType} # These are either Unitful quantities, which are uniformly supported # across all of AtomsBase or the value has a type that Extxyz can write @@ -119,9 +113,9 @@ function Atoms(dict::Dict{String, Any}) atom_data[:atomic_symbol] = [Symbol(element(num).symbol) for num in Z] end if haskey(arrays, "mass") - atom_data[:atomic_mass] = arrays["mass"]u"u" + atom_data[:mass] = arrays["mass"]u"u" else - atom_data[:atomic_mass] = [element(num).atomic_mass for num in Z] + atom_data[:mass] = [element(num).atomic_mass for num in Z] end if haskey(arrays, "velocities") atom_data[:velocity] = collect(eachcol(arrays["velocities"])) * uVelocity @@ -146,16 +140,17 @@ function Atoms(dict::Dict{String, Any}) if haskey(dict, "cell") system_data[:bounding_box] = collect(eachrow(dict["cell"]))u"Å" if haskey(dict, "pbc") - system_data[:boundary_conditions] = [p ? Periodic() : DirichletZero() - for p in dict["pbc"]] + system_data[:periodicity] = tuple(dict["pbc"]...) else @warn "'pbc' not contained in dict. Defaulting to all-periodic boundary. " - system_data[:boundary_conditions] = fill(Periodic(), 3) + system_data[:periodicity] = (true, true, true) end else # Infinite system haskey(dict, "pbc") && @warn "'pbc' ignored since no 'cell' entry found in dict." - system_data[:boundary_conditions] = fill(DirichletZero(), 3) - system_data[:bounding_box] = infinite_box(3) + system_data[:periodicity] = (false, false, false) + system_data[:bounding_box] = ( SVector(Inf, 0.0, 0.0), + SVector(0.0, Inf, 0.0), + SVector(0.0, 0.0, Inf) ) end for key in keys(info) @@ -168,6 +163,7 @@ function Atoms(dict::Dict{String, Any}) Atoms(NamedTuple(atom_data), NamedTuple(system_data)) end + read_dict(dict::Dict{String,Any}) = Atoms(dict) function write_dict(atoms::Atoms) @@ -180,7 +176,7 @@ function write_dict(atoms::Atoms) "in ExtXYZ. Atomic numbers take preference.") end if atoms.atom_data.atomic_mass != [element(Z).atomic_mass for Z in arrays["Z"]] - arrays["mass"] = ustrip.(u"u", atoms.atom_data.atomic_mass) + arrays["mass"] = ustrip.(u"u", atoms.atom_data.mass) end arrays["velocities"] = zeros(D, length(atoms)) @@ -193,7 +189,7 @@ function write_dict(atoms::Atoms) end for (k, v) in pairs(atoms.atom_data) - k in (:atomic_mass, :atomic_symbol, :atomic_number, :position, :velocity) && continue + k in (:mass, :atomic_symbol, :atomic_number, :position, :velocity) && continue if k in (:vdw_radius, :covalent_radius) # Remove length unit arrays[string(k)] = ustrip.(u"Å", v) elseif k in (:charge, ) @@ -207,10 +203,7 @@ function write_dict(atoms::Atoms) end end - pbc = zeros(Bool, D) - for (i, bc) in enumerate(atoms.system_data.boundary_conditions) - pbc[i] = bc isa Periodic - end + pbc = atoms.system_data.periodicity cell = zeros(D, D) for (i, bvector) in enumerate(atoms.system_data.bounding_box) cell[i, :] = ustrip.(u"Å", bvector) @@ -219,7 +212,7 @@ function write_dict(atoms::Atoms) # Deal with other system keys info = Dict{String,Any}() for (k, v) in pairs(atoms.system_data) - k in (:boundary_conditions, :bounding_box) && continue # Already dealt with + k in (:periodicity, :bounding_box) && continue # Already dealt with if k in (:charge, ) info[string(k)] = ustrip(u"e_au", atoms.system_data[k]) elseif v isa ExtxyzType @@ -249,9 +242,14 @@ write_dict(system::AbstractSystem{D}) = write_dict(Atoms(system)) Base.length(sys::Atoms) = length(sys.atom_data.position) Base.size(sys::Atoms) = (length(sys), ) AtomsBase.bounding_box(sys::Atoms) = sys.system_data.bounding_box -AtomsBase.boundary_conditions(sys::Atoms) = sys.system_data.boundary_conditions +AtomsBase.periodicity(sys::Atoms) = sys.system_data.periodicity + +# AtomsBase now requires a cell object to be returned instead of bounding_box +# and boundary conditions. But this can just be constructed on the fly. +AtomsBase.cell(sys::Atoms) = AtomsBase.PeriodicCell(; + cell_vectors = sys.system_data.bounding_box, + periodicity = sys.system_data.periodicity ) -AtomsBase.species_type(::FS) where {FS <: Atoms} = AtomView{FS} Base.getindex(sys::Atoms, x::Symbol) = getindex(sys.system_data, x) Base.haskey(sys::Atoms, x::Symbol) = haskey(sys.system_data, x) Base.keys(sys::Atoms) = keys(sys.system_data) @@ -263,16 +261,17 @@ Base.getindex(sys::Atoms, ::Colon, x::Symbol) = getindex(sys.atom_data, x) AtomsBase.atomkeys(sys::Atoms) = keys(sys.atom_data) AtomsBase.hasatomkey(sys::Atoms, x::Symbol) = haskey(sys.atom_data, x) -AtomsBase.position(s::Atoms) = Base.getindex(s, :, :position) -AtomsBase.position(s::Atoms, i::Integer) = Base.getindex(s, i, :position) -AtomsBase.velocity(s::Atoms) = Base.getindex(s, :, :velocity) -AtomsBase.velocity(s::Atoms, i::Integer) = Base.getindex(s, i, :velocity) -AtomsBase.atomic_mass(s::Atoms) = Base.getindex(s, :, :atomic_mass) -AtomsBase.atomic_mass(s::Atoms, i::Integer) = Base.getindex(s, i, :atomic_mass) -AtomsBase.atomic_symbol(s::Atoms) = Base.getindex(s, :, :atomic_symbol) -AtomsBase.atomic_symbol(s::Atoms, i::Integer) = Base.getindex(s, i, :atomic_symbol) -AtomsBase.atomic_number(s::Atoms) = Base.getindex(s, :, :atomic_number) -AtomsBase.atomic_number(s::Atoms, i::Integer) = Base.getindex(s, i, :atomic_number) +const _IDX = Union{Colon, Integer, AbstractArray{<: Integer}} +AtomsBase.position(s::Atoms, i::_IDX) = getindex(s, i, :position) +AtomsBase.velocity(s::Atoms, i::_IDX) = getindex(s, i, :velocity) +AtomsBase.mass(s::Atoms, i::_IDX) = getindex(s, i, :mass) +AtomsBase.atomic_symbol(s::Atoms, i::_IDX) = getindex(s, i, :atomic_symbol) +AtomsBase.atomic_symbol(s::Atoms, i::_IDX) = getindex(s, i, :atomic_number) + +# AtomsBase now requires the `species` function to be implemented. Since +# ExtXYZ requires that atoms are uniquely identified by their atomic number, we +# will use the atomic number as the species identifier. +AtomsBase.species(s::Atoms, i::_IDX) = AtomsBase.atomic_number(s, i) # --------- FileIO compatible interface (hence not exported) From e837828805265729855364b9fac265d98d7b3ff4 Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Mon, 2 Sep 2024 12:25:33 -0700 Subject: [PATCH 2/6] debugging... --- Project.toml | 5 +++-- src/atoms.jl | 31 ++++++++++++++++++++----------- test/Project.toml | 2 +- test/atomsbase.jl | 9 ++++++++- 4 files changed, 32 insertions(+), 15 deletions(-) diff --git a/Project.toml b/Project.toml index 626a4f5..46f5108 100644 --- a/Project.toml +++ b/Project.toml @@ -1,10 +1,11 @@ name = "ExtXYZ" uuid = "352459e4-ddd7-4360-8937-99dcb397b478" authors = ["James Kermode and contributors"] -version = "0.2-DEV" +version = "0.2.0-DEV" [deps] AtomsBase = "a963bdd2-2df7-4f54-a1ee-49d51e6be12a" +AtomsBaseTesting = "ed7c10db-df7e-4efa-a7be-4f4190f7f227" PeriodicTable = "7b2266bf-644c-5ea3-82d8-af4bbd25a884" StaticArrays = "90137ffa-7385-5640-81b9-e52037218182" Unitful = "1986cc42-f94f-5a68-af5c-568840ba703d" @@ -17,8 +18,8 @@ PeriodicTable = "1" StaticArrays = "1.5" Unitful = "1" UnitfulAtomic = "1" -julia = "1" extxyz_jll = "0.1.3" +julia = "1" [extras] Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" diff --git a/src/atoms.jl b/src/atoms.jl index dcd6279..4199d5f 100644 --- a/src/atoms.jl +++ b/src/atoms.jl @@ -1,7 +1,9 @@ using AtomsBase using Unitful using UnitfulAtomic +using StaticArrays +import AtomsBase: AbstractSystem export Atoms const D = 3 # TODO generalise to arbitrary spatial dimensions @@ -24,14 +26,14 @@ end function Atoms(system::AbstractSystem{D}) n_atoms = length(system) atomic_symbols = [Symbol(element(atomic_number(at)).symbol) for at in system] - if atomic_symbols != atomic_symbol(system, :) + if atomic_symbols != Symbol.(atomic_symbol(system, :)) @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") end atom_data = Dict{Symbol,Any}( :atomic_symbol => atomic_symbols, - :atomic_number => atomic_number(system), - :mass => mass(system) + :atomic_number => Int.(atomic_number(system, :)), # gets messy if not Int + :mass => mass(system, :) ) atom_data[:position] = map(1:n_atoms) do at pos = zeros(3)u"Å" @@ -40,17 +42,18 @@ function Atoms(system::AbstractSystem{D}) end atom_data[:velocity] = map(1:n_atoms) do at vel = zeros(3) * uVelocity - if !ismissing(velocity(system)) && !ismissing(velocity(system, at)) + if !ismissing(velocity(system, :)) && !ismissing(velocity(system, at)) vel[1:D] = velocity(system, at) end - SVector{D, eltype(pos)}(vel) # AtomsBase 0.4 requires SVector + SVector{D, eltype(vel)}(vel) # AtomsBase 0.4 requires SVector end for k in atomkeys(system) if k in (:atomic_symbol, :atomic_number, :mass, :velocity, :position) continue # Already done end - atoms_base_keys = (:charge, :covalent_radius, :vdw_radius, + # atomic_mass is deprecated for but is sometimes still used + atoms_base_keys = (:charge, :atomic_mass, :covalent_radius, :vdw_radius, :magnetic_moment, :pseudopotential) v = system[1, k] if k in atoms_base_keys || v isa ExtxyzType || v isa AbstractVector{<: ExtxyzType} @@ -60,14 +63,18 @@ function Atoms(system::AbstractSystem{D}) atom_data[k] = system[:, k] elseif v isa Quantity || (v isa AbstractArray && eltype(v) <: Quantity) @warn "Unitful quantity $k is not yet supported in ExtXYZ." + elseif k == :species + # atomic_number represents the species in ExtXYZ, which is already + # written into the atom_data dictionary + continue else @warn "Writing quantities of type $(typeof(v)) is not supported in ExtXYZ." end end system_data = Dict{Symbol,Any}( - :bounding_box => bounding_box(system) # NTuple{D, SVector}, - :periodicity => periodicity(system) # NTuple{D, Bool} + :bounding_box => bounding_box(system), + :periodicity => periodicity(system) ) # Extract extra system properties @@ -175,7 +182,7 @@ function write_dict(atoms::Atoms) @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") end - if atoms.atom_data.atomic_mass != [element(Z).atomic_mass for Z in arrays["Z"]] + if atoms.atom_data.mass != [element(Z).atomic_mass for Z in arrays["Z"]] arrays["mass"] = ustrip.(u"u", atoms.atom_data.mass) end @@ -256,6 +263,7 @@ Base.keys(sys::Atoms) = keys(sys.system_data) Base.getindex(sys::Atoms, i::Integer) = AtomView(sys, i) Base.getindex(sys::Atoms, i::Integer, x::Symbol) = getindex(sys.atom_data, x)[i] +Base.getindex(sys::Atoms, i::AbstractVector{<: Integer}, x::Symbol) = getindex(sys.atom_data, x)[i] Base.getindex(sys::Atoms, ::Colon, x::Symbol) = getindex(sys.atom_data, x) AtomsBase.atomkeys(sys::Atoms) = keys(sys.atom_data) @@ -266,12 +274,13 @@ AtomsBase.position(s::Atoms, i::_IDX) = getindex(s, i, :position) AtomsBase.velocity(s::Atoms, i::_IDX) = getindex(s, i, :velocity) AtomsBase.mass(s::Atoms, i::_IDX) = getindex(s, i, :mass) AtomsBase.atomic_symbol(s::Atoms, i::_IDX) = getindex(s, i, :atomic_symbol) -AtomsBase.atomic_symbol(s::Atoms, i::_IDX) = getindex(s, i, :atomic_number) +AtomsBase.atomic_number(s::Atoms, i::_IDX) = getindex(s, i, :atomic_number) # AtomsBase now requires the `species` function to be implemented. Since # ExtXYZ requires that atoms are uniquely identified by their atomic number, we # will use the atomic number as the species identifier. -AtomsBase.species(s::Atoms, i::_IDX) = AtomsBase.atomic_number(s, i) +AtomsBase.species(s::Atoms, i::_IDX) = + AtomsBase.ChemicalSpecies.(AtomsBase.atomic_symbol(s, i)) # --------- FileIO compatible interface (hence not exported) diff --git a/test/Project.toml b/test/Project.toml index c1eb0f9..5eacf23 100644 --- a/test/Project.toml +++ b/test/Project.toml @@ -1,5 +1,5 @@ [compat] -AtomsBaseTesting = "0.1" +AtomsBaseTesting = "0.2" [deps] AtomsBase = "a963bdd2-2df7-4f54-a1ee-49d51e6be12a" diff --git a/test/atomsbase.jl b/test/atomsbase.jl index c3d3d6d..4ffc67e 100644 --- a/test/atomsbase.jl +++ b/test/atomsbase.jl @@ -5,6 +5,13 @@ using Test using Unitful using UnitfulAtomic +AtomsBase.atomic_number(z::Integer) = z +AtomsBase.atomic_symbol(z::Integer) = AtomsBase._chem_el_info[z].symbol + +system = make_test_system().system +at_sys = Atoms(system) +test_approx_eq(system, Atoms(system)) + @testset "Conversion AtomsBase -> Atoms" begin system = make_test_system().system test_approx_eq(system, Atoms(system)) @@ -33,7 +40,7 @@ end arrays = atoms["arrays"] @test arrays["Z"] == atprop.atomic_number @test arrays["species"] == string.(atprop.atomic_symbol) - @test arrays["mass"] == ustrip.(u"u", atprop.atomic_mass) + @test arrays["mass"] == ustrip.(u"u", atprop.mass) @test arrays["pos"] ≈ ustrip.(u"Å", hcat(atprop.position...)) atol=1e-10 @test arrays["velocities"] ≈ ustrip.(sqrt(u"eV"/u"u"), hcat(atprop.velocity...)) atol=1e-10 From 17f5b7816370a7d852c8e1daa4a461f2693792f7 Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Mon, 2 Sep 2024 16:14:59 -0700 Subject: [PATCH 3/6] fixed several tests --- src/atoms.jl | 18 +++++++++--------- test/atomsbase.jl | 44 +++++++++++++++++++++++++++----------------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/atoms.jl b/src/atoms.jl index 4199d5f..41ebd50 100644 --- a/src/atoms.jl +++ b/src/atoms.jl @@ -25,14 +25,17 @@ end function Atoms(system::AbstractSystem{D}) n_atoms = length(system) + s = species(system, :) atomic_symbols = [Symbol(element(atomic_number(at)).symbol) for at in system] - if atomic_symbols != Symbol.(atomic_symbol(system, :)) + atomic_numbers = atomic_number.(s) + if atomic_symbols != Symbol.(s) @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") end atom_data = Dict{Symbol,Any}( :atomic_symbol => atomic_symbols, :atomic_number => Int.(atomic_number(system, :)), # gets messy if not Int + :species => s, :mass => mass(system, :) ) atom_data[:position] = map(1:n_atoms) do at @@ -49,7 +52,7 @@ function Atoms(system::AbstractSystem{D}) end for k in atomkeys(system) - if k in (:atomic_symbol, :atomic_number, :mass, :velocity, :position) + if k in (:species, :atomic_symbol, :atomic_number, :mass, :velocity, :position) continue # Already done end # atomic_mass is deprecated for but is sometimes still used @@ -63,10 +66,6 @@ function Atoms(system::AbstractSystem{D}) atom_data[k] = system[:, k] elseif v isa Quantity || (v isa AbstractArray && eltype(v) <: Quantity) @warn "Unitful quantity $k is not yet supported in ExtXYZ." - elseif k == :species - # atomic_number represents the species in ExtXYZ, which is already - # written into the atom_data dictionary - continue else @warn "Writing quantities of type $(typeof(v)) is not supported in ExtXYZ." end @@ -177,7 +176,7 @@ function write_dict(atoms::Atoms) arrays = Dict{String,Any}() arrays["Z"] = atoms.atom_data.atomic_number - arrays["species"] = [element(Z).symbol for Z in arrays["Z"]] + arrays["atomic_symbol"] = [element(Z).symbol for Z in arrays["Z"]] if atoms.atom_data.atomic_symbol != [Symbol(element(Z).symbol) for Z in arrays["Z"]] @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") @@ -196,7 +195,8 @@ function write_dict(atoms::Atoms) end for (k, v) in pairs(atoms.atom_data) - k in (:mass, :atomic_symbol, :atomic_number, :position, :velocity) && continue + k in (:mass, :atomic_mass, :atomic_symbol, :atomic_number, :position, + :velocity, :species) && continue if k in (:vdw_radius, :covalent_radius) # Remove length unit arrays[string(k)] = ustrip.(u"Å", v) elseif k in (:charge, ) @@ -236,7 +236,7 @@ function write_dict(atoms::Atoms) end end dict = Dict("N_atoms" => length(atoms), - "pbc" => pbc, + "pbc" => [pbc...], "info" => info, "arrays" => arrays) all(cell .!= Inf) && (dict["cell"] = cell) # only write cell if its finite diff --git a/test/atomsbase.jl b/test/atomsbase.jl index 4ffc67e..d11d6da 100644 --- a/test/atomsbase.jl +++ b/test/atomsbase.jl @@ -7,29 +7,38 @@ using UnitfulAtomic AtomsBase.atomic_number(z::Integer) = z AtomsBase.atomic_symbol(z::Integer) = AtomsBase._chem_el_info[z].symbol - -system = make_test_system().system -at_sys = Atoms(system) -test_approx_eq(system, Atoms(system)) +AtomsBase.atomic_number(s::Symbol) = AtomsBase._sym2z[s] + +function simple_test_approx_eq(sys1, sys2) + @test all(position(sys1, :) .≈ position(sys2, :)) + @test all(velocity(sys1, :) .≈ velocity(sys2, :)) + @test mass(sys1, :) ≈ mass(sys2, :) + @test species(sys1, :) == species(sys2, :) + @test atomic_number(sys1, :) == atomic_number(sys2, :) + @test atomic_symbol(sys1, :) == atomic_symbol(sys2, :) + @test cell(sys1) == cell(sys2) + @test periodicity(sys1) == periodicity(sys2) + @test all(bounding_box(sys1) .≈ bounding_box(sys2)) +end @testset "Conversion AtomsBase -> Atoms" begin system = make_test_system().system - test_approx_eq(system, Atoms(system)) + simple_test_approx_eq(system, Atoms(system)) end @testset "Conversion AtomsBase -> dict (velocity)" begin system, atoms, atprop, sysprop, box, bcs = make_test_system() atoms = ExtXYZ.write_dict(Atoms(system)) - cell = zeros(3, 3) + c3ll = zeros(3, 3) for i in 1:3 - cell[i, :] = ustrip.(u"Å", box[i]) + c3ll[i, :] = ustrip.(u"Å", box[i]) end - @assert bcs == [Periodic(), Periodic(), DirichletZero()] + @assert bcs == (true, true, false) @test atoms["pbc"] == [true, true, false] @test atoms["N_atoms"] == 5 - @test atoms["cell"] == cell + @test atoms["cell"] == c3ll info = atoms["info"] @test sort(collect(keys(info))) == ["charge", "extra_data", "multiplicity"] @@ -38,15 +47,16 @@ end @test info["multiplicity"] == sysprop.multiplicity arrays = atoms["arrays"] - @test arrays["Z"] == atprop.atomic_number - @test arrays["species"] == string.(atprop.atomic_symbol) - @test arrays["mass"] == ustrip.(u"u", atprop.mass) + @test arrays["Z"] == AtomsBase.atomic_number.(atprop.atomic_symbol) + @test arrays["atomic_symbol"] == string.(atprop.atomic_symbol) + # mass is not written to the dict because the mass == mass(element) + # @test arrays["mass"] == ustrip.(u"u", atprop.atomic_mass) @test arrays["pos"] ≈ ustrip.(u"Å", hcat(atprop.position...)) atol=1e-10 @test arrays["velocities"] ≈ ustrip.(sqrt(u"eV"/u"u"), hcat(atprop.velocity...)) atol=1e-10 - expected_atkeys = ["Z", "charge", "covalent_radius", "magnetic_moment", - "mass", "pos", "species", "vdw_radius", "velocities"] + expected_atkeys = ["Z", "atomic_symbol", "charge", "covalent_radius", + "magnetic_moment", "pos", "vdw_radius", "velocities"] @test sort(collect(keys(arrays))) == expected_atkeys @test arrays["magnetic_moment"] == atprop.magnetic_moment @test arrays["vdw_radius"] == ustrip.(u"Å", atprop.vdw_radius) @@ -68,7 +78,7 @@ end @test length(atoms) == 1 @test all(periodicity(atoms)) @test atomic_symbol(atoms, 1) == :H - @test atomic_number(atoms) == [1] + @test atomic_number(atoms, :) == [1,] @test iszero(velocity(atoms, 1)) end @@ -82,7 +92,7 @@ end (:warn, r"Mismatch between atomic numbers and atomic symbols"), match_mode=:any, ExtXYZ.write_dict(Atoms(system))) - @test atoms["arrays"]["species"] == ["H", "H", "C", "N", "He"] + @test atoms["arrays"]["atomic_symbol"] == ["H", "H", "C", "N", "He"] @test atoms["arrays"]["Z"] == [1, 1, 6, 7, 2] end @@ -93,7 +103,7 @@ end ExtXYZ.save(outfile, system) ExtXYZ.load(outfile)::AbstractSystem end - test_approx_eq(system, io_system; rtol=1e-4) + simple_test_approx_eq(system, io_system; rtol=1e-4) end @testset "Extra variables for atoms" begin From bfe24f3b972353edd27049ee5a6e51e64c08190f Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Mon, 2 Sep 2024 21:13:37 -0700 Subject: [PATCH 4/6] tests pass --- src/atoms.jl | 27 ++++++++++++++++----------- src/fileio.jl | 2 +- test/atomsbase.jl | 29 +++++++++++++++++++---------- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/atoms.jl b/src/atoms.jl index 41ebd50..fcddfec 100644 --- a/src/atoms.jl +++ b/src/atoms.jl @@ -104,20 +104,25 @@ function Atoms(dict::Dict{String, Any}) elseif haskey(arrays, "species") Z = [element(Symbol(spec)).number for spec in arrays["species"]] else - error("Cannot determine atomic numbers. Either 'Z' or 'species' must " * + error("Cannot determine atomic numbers. Either 'Z' or 'S' must " * "be present in arrays") end @assert length(Z) == dict["N_atoms"] + atomic_symbols = [Symbol(element(num).symbol) for num in Z] atom_data = Dict{Symbol, Any}( :position => collect(eachcol(arrays["pos"]))u"Å", :atomic_number => Z, + :atomic_symbol => atomic_symbols, + :species => AtomsBase.ChemicalSpecies.(atomic_symbols) ) - if haskey(arrays, "species") - atom_data[:atomic_symbol] = Symbol.(arrays["species"]) - else - atom_data[:atomic_symbol] = [Symbol(element(num).symbol) for num in Z] - end + # TODO; Instead of the following, should there be a consistency check + # between S and Z? + # if haskey(arrays, "species") + # atom_data[:atomic_symbol] = Symbol.(arrays["species"]) + # else + # atom_data[:atomic_symbol] = [Symbol(element(num).symbol) for num in Z] + # end if haskey(arrays, "mass") atom_data[:mass] = arrays["mass"]u"u" else @@ -130,7 +135,7 @@ function Atoms(dict::Dict{String, Any}) end for key in keys(arrays) - key in ("mass", "species", "Z", "pos", "velocities") && continue # Already done + key in ("mass", "species", "Z", "atomic_symbol", "pos", "velocities") && continue # Already done if key in ("vdw_radius", "covalent_radius") # Add length unit atom_data[Symbol(key)] = arrays[key] * u"Å" elseif key in ("charge", ) # Add charge unit @@ -154,9 +159,9 @@ function Atoms(dict::Dict{String, Any}) else # Infinite system haskey(dict, "pbc") && @warn "'pbc' ignored since no 'cell' entry found in dict." system_data[:periodicity] = (false, false, false) - system_data[:bounding_box] = ( SVector(Inf, 0.0, 0.0), - SVector(0.0, Inf, 0.0), - SVector(0.0, 0.0, Inf) ) + system_data[:bounding_box] = ( SVector(Inf, 0.0, 0.0) * u"Å", + SVector(0.0, Inf, 0.0) * u"Å", + SVector(0.0, 0.0, Inf) * u"Å" ) end for key in keys(info) @@ -176,7 +181,7 @@ function write_dict(atoms::Atoms) arrays = Dict{String,Any}() arrays["Z"] = atoms.atom_data.atomic_number - arrays["atomic_symbol"] = [element(Z).symbol for Z in arrays["Z"]] + arrays["species"] = [element(Z).symbol for Z in arrays["Z"]] if atoms.atom_data.atomic_symbol != [Symbol(element(Z).symbol) for Z in arrays["Z"]] @warn("Mismatch between atomic numbers and atomic symbols, which is not supported " * "in ExtXYZ. Atomic numbers take preference.") diff --git a/src/fileio.jl b/src/fileio.jl index 5fd6452..fadf78e 100644 --- a/src/fileio.jl +++ b/src/fileio.jl @@ -359,7 +359,7 @@ function write_frame_dicts(fp::Ptr{Cvoid}, nat, info, arrays; verbose=false) nat = Cint(nat) cinfo = convert(Ptr{DictEntry}, info; transpose_arrays=true) - # ensure "species" goes in column 1 and "pos" goes in column 2 + # ensure "species" (symbol!) goes in column 1 and "pos" goes in column 2 ordered_keys = collect(keys(arrays)) species_idx = findfirst(isequal("species"), ordered_keys) ordered_keys[1], ordered_keys[species_idx] = ordered_keys[species_idx], ordered_keys[1] diff --git a/test/atomsbase.jl b/test/atomsbase.jl index d11d6da..6d8396a 100644 --- a/test/atomsbase.jl +++ b/test/atomsbase.jl @@ -4,21 +4,24 @@ using ExtXYZ using Test using Unitful using UnitfulAtomic +using AtomsBase: AbstractSystem -AtomsBase.atomic_number(z::Integer) = z +# AtomsBase.atomic_number(z::Integer) = z AtomsBase.atomic_symbol(z::Integer) = AtomsBase._chem_el_info[z].symbol AtomsBase.atomic_number(s::Symbol) = AtomsBase._sym2z[s] -function simple_test_approx_eq(sys1, sys2) +function simple_test_approx_eq(sys1, sys2; test_cell = true) @test all(position(sys1, :) .≈ position(sys2, :)) @test all(velocity(sys1, :) .≈ velocity(sys2, :)) @test mass(sys1, :) ≈ mass(sys2, :) @test species(sys1, :) == species(sys2, :) @test atomic_number(sys1, :) == atomic_number(sys2, :) @test atomic_symbol(sys1, :) == atomic_symbol(sys2, :) - @test cell(sys1) == cell(sys2) @test periodicity(sys1) == periodicity(sys2) @test all(bounding_box(sys1) .≈ bounding_box(sys2)) + if test_cell + @test cell(sys1) == cell(sys2) + end end @testset "Conversion AtomsBase -> Atoms" begin @@ -48,16 +51,16 @@ end arrays = atoms["arrays"] @test arrays["Z"] == AtomsBase.atomic_number.(atprop.atomic_symbol) - @test arrays["atomic_symbol"] == string.(atprop.atomic_symbol) + @test arrays["species"] == string.(atprop.atomic_symbol) # mass is not written to the dict because the mass == mass(element) # @test arrays["mass"] == ustrip.(u"u", atprop.atomic_mass) @test arrays["pos"] ≈ ustrip.(u"Å", hcat(atprop.position...)) atol=1e-10 @test arrays["velocities"] ≈ ustrip.(sqrt(u"eV"/u"u"), hcat(atprop.velocity...)) atol=1e-10 - expected_atkeys = ["Z", "atomic_symbol", "charge", "covalent_radius", + expected_atkeys = ["Z", "species", "charge", "covalent_radius", "magnetic_moment", "pos", "vdw_radius", "velocities"] - @test sort(collect(keys(arrays))) == expected_atkeys + @test sort(collect(keys(arrays))) == sort(expected_atkeys) @test arrays["magnetic_moment"] == atprop.magnetic_moment @test arrays["vdw_radius"] == ustrip.(u"Å", atprop.vdw_radius) @test arrays["covalent_radius"] == ustrip.(u"Å", atprop.covalent_radius) @@ -92,7 +95,7 @@ end (:warn, r"Mismatch between atomic numbers and atomic symbols"), match_mode=:any, ExtXYZ.write_dict(Atoms(system))) - @test atoms["arrays"]["atomic_symbol"] == ["H", "H", "C", "N", "He"] + @test atoms["arrays"]["species"] == ["H", "H", "C", "N", "He"] @test atoms["arrays"]["Z"] == [1, 1, 6, 7, 2] end @@ -103,7 +106,8 @@ end ExtXYZ.save(outfile, system) ExtXYZ.load(outfile)::AbstractSystem end - simple_test_approx_eq(system, io_system; rtol=1e-4) + # test_approx_eq(system, io_system; rtol=1e-4) + simple_test_approx_eq(system, io_system) end @testset "Extra variables for atoms" begin @@ -162,7 +166,9 @@ end end end - +# TODO: This test is failing because ExtXYZ doesn't store a general cell, but +# always stores the cell vectors. Because of this, the equality test +# cannot pass. @testset "AtomsBase isolated system" begin hydrogen = isolated_system([ :H => [0, 0, 0.]u"Å", @@ -173,7 +179,10 @@ end try ExtXYZ.save(fname, hydrogen) new_sys = ExtXYZ.load(fname) - test_approx_eq(hydrogen, new_sys; rtol=1e-4) + # test_approx_eq(hydrogen, new_sys; rtol=1e-4) + # note that test_cell = false only removes the equality test for + # the cell object, it still tests equality of the cell vectors and pbc + simple_test_approx_eq(hydrogen, new_sys; test_cell=false) finally isfile(fname) && rm(fname) end From b74f6c8b1d7f91ee2f15db27a1e0fc00ae64f3e0 Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Wed, 4 Sep 2024 09:32:45 -0700 Subject: [PATCH 5/6] remove monkey-patches --- Project.toml | 4 ++-- test/atomsbase.jl | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/Project.toml b/Project.toml index 46f5108..25094b0 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "ExtXYZ" uuid = "352459e4-ddd7-4360-8937-99dcb397b478" authors = ["James Kermode and contributors"] -version = "0.2.0-DEV" +version = "0.2.0-dev" [deps] AtomsBase = "a963bdd2-2df7-4f54-a1ee-49d51e6be12a" @@ -13,7 +13,7 @@ UnitfulAtomic = "a7773ee8-282e-5fa2-be4e-bd808c38a91a" extxyz_jll = "6ecdc6fc-93a8-5528-aee3-ac7ae1c60be7" [compat] -AtomsBase = "0.3, 0.4" +AtomsBase = "0.4" PeriodicTable = "1" StaticArrays = "1.5" Unitful = "1" diff --git a/test/atomsbase.jl b/test/atomsbase.jl index 6d8396a..41be500 100644 --- a/test/atomsbase.jl +++ b/test/atomsbase.jl @@ -6,9 +6,6 @@ using Unitful using UnitfulAtomic using AtomsBase: AbstractSystem -# AtomsBase.atomic_number(z::Integer) = z -AtomsBase.atomic_symbol(z::Integer) = AtomsBase._chem_el_info[z].symbol -AtomsBase.atomic_number(s::Symbol) = AtomsBase._sym2z[s] function simple_test_approx_eq(sys1, sys2; test_cell = true) @test all(position(sys1, :) .≈ position(sys2, :)) From a5c7d6695183f48fd3a59db688f7af5267d25cc7 Mon Sep 17 00:00:00 2001 From: Christoph Ortner Date: Mon, 9 Sep 2024 12:08:17 -0700 Subject: [PATCH 6/6] fix AtomsBase version --- Project.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Project.toml b/Project.toml index 25094b0..63c0975 100644 --- a/Project.toml +++ b/Project.toml @@ -13,7 +13,7 @@ UnitfulAtomic = "a7773ee8-282e-5fa2-be4e-bd808c38a91a" extxyz_jll = "6ecdc6fc-93a8-5528-aee3-ac7ae1c60be7" [compat] -AtomsBase = "0.4" +AtomsBase = "0.4.1" PeriodicTable = "1" StaticArrays = "1.5" Unitful = "1"