libAtoms / ExtXYZ.jl

Extended XYZ read/write support for Julia
MIT License
13 stars 6 forks source link

Missing PBC handled badly? #54

Closed cortner closed 1 month ago

cortner commented 1 month ago
using ExtXYZ 

download("https://www.dropbox.com/scl/fi/z6lvcpx3djp775zenz032/Si-PRX-2018.xyz?rlkey=ja5e9z99c3ta1ugra5ayq5lcv&st=cs6g7vbu&dl=1",
         "Si_dataset.xyz");

Si_dataset = ExtXYZ.load("Si_dataset.xyz");

results in

┌ Warning: 'pbc' not contained in dict. Defaulting to all-periodic boundary. 
└ @ ExtXYZ ~/.julia/packages/ExtXYZ/dBeGN/src/atoms.jl:152

which refers to

Si_dataset[1].system_data.periodicity
# (true, true, true)

CC @wcwitt

cortner commented 1 month ago

Versions:

Status `~/gits/ACEpotentials.jl/examples/Tutorial/Project.toml`
  [3b96b61c] ACEpotentials v0.8.0-dev `../..`
  [352459e4] ExtXYZ v0.2.0
  [7073ff75] IJulia v1.25.0
  [b964fa9f] LaTeXStrings v1.3.1
  [6f286f6a] MultivariateStats v0.10.3
  [91a5bcdd] Plots v1.40.8
  [08abe8d2] PrettyTables v2.3.2
  [fd094767] Suppressor v0.2.8
wcwitt commented 1 month ago

Hm, what default would you want in this case?

I think if a lattice is provided, the extxyz spec says PBC is assumed.

On Sat, 14 Sep 2024 at 14:26, Christoph Ortner @.***> wrote:

using ExtXYZ

download("https://www.dropbox.com/scl/fi/z6lvcpx3djp775zenz032/Si-PRX-2018.xyz?rlkey=ja5e9z99c3ta1ugra5ayq5lcv&st=cs6g7vbu&dl=1", "Si_dataset.xyz");

Si_dataset = ExtXYZ.load("Si_dataset.xyz");

results in

┌ Warning: 'pbc' not contained in dict. Defaulting to all-periodic boundary. └ @ ExtXYZ ~/.julia/packages/ExtXYZ/dBeGN/src/atoms.jl:152

which refers to

Si_dataset[1].system_data.periodicity

(true, true, true)

CC @wcwitt https://github.com/wcwitt

— Reply to this email directly, view it on GitHub https://github.com/libAtoms/ExtXYZ.jl/issues/54, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXHHTQ3JB4QEYULAVWYQC3ZWR5ULAVCNFSM6AAAAABOHB35QOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGUZDMNJVGMYTKNA . You are receiving this because you were mentioned.Message ID: @.***>

cortner commented 1 month ago

I see - I shouldbhave checked that. So the dataset is problematic not the loader?

jameskermode commented 1 month ago

I agree with Chuck that current behaviour matches spec at https://github.com/libAtoms/extxyz. If lattice is missing then we default to open boundaries, but if it's present the pbc is the correct default.

cortner commented 1 month ago

Ok - I'll just have to manually fix the dataset then. For now I've just removed that system which I think is the correct thing anyhow.

cortner commented 1 month ago

Thanks for the quick response.