JuliaMolSim / AtomsBase.jl

A Julian abstract interface for atomic structures.
https://juliamolsim.github.io/AtomsBase.jl/
MIT License
84 stars 18 forks source link

`periodic_system` tutorial is broken #117

Closed lxvm closed 2 months ago

lxvm commented 2 months ago

Hi, I was trying out AtomsBase.jl v0.4 and found that some of the docstrings are out of date. For example, in periodic_system the docstring recommends the following

using Unitful
using UnitfulAtomic
using AtomsBase
bounding_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],
                                 bounding_box, fractional=true)

First, bounding_box is now exported, so perhaps rename the variable to bbox? Second, the call to periodic system errors with

ERROR: MethodError: no method matching FlexibleSystem(::Vector{Atom{3, Quantity{…}, Quantity{…}, Quantity{…}}}, ::Vector{Vector{Quantity{…}}}, ::Vector{Bool})

Closest candidates are:
  FlexibleSystem(::AbstractVector{S}, ::Tuple{Vararg{var"#s15", D}} where var"#s15"<:AbstractVector{L}, ::Union{Bool, Tuple{Vararg{Bool, D}}, AbstractVector{<:Bool}}; kwargs...) where {L<:(Union{Quantity{T, 𝐋, U}, Level{L, S, Quantity{T, 𝐋, U}} where {L, S}} where {T, U}), S, D}
   @ AtomsBase ~/.julia/packages/AtomsBase/4AWmO/src/implementation/flexible_system.jl:51
  FlexibleSystem(::AbstractVector, ::Any; kwargs...)
   @ AtomsBase ~/.julia/packages/AtomsBase/4AWmO/src/implementation/flexible_system.jl:73
  FlexibleSystem(::Any; bounding_box, periodicity, kwargs...)
   @ AtomsBase ~/.julia/packages/AtomsBase/4AWmO/src/implementation/flexible_system.jl:69

Stacktrace:
 [1] atomic_system(atoms::Vector{Atom{…}}, box::Vector{Vector{…}}, bcs::Vector{Bool}; kwargs::@Kwargs{})
   @ AtomsBase ~/.julia/packages/AtomsBase/4AWmO/src/implementation/utils.jl:25
 [2] periodic_system(atoms::Vector{Pair{…}}, box::Vector{Vector{…}}; fractional::Bool, kwargs::@Kwargs{})
   @ AtomsBase ~/.julia/packages/AtomsBase/4AWmO/src/implementation/utils.jl:92
 [3] top-level scope
   @ REPL[17]:1
Some type information was truncated. Use `show(err)` to see complete types.

This seems to be fixed by instead defining

bounding_box = 10.26 / 2 .* ([0, 0, 1], [1, 0, 1], [1, 1, 0]) .* u"bohr"

I gather that there are several breaking API changes in v0.4. Is there a summary of them in some place?

cortner commented 2 months ago

Thanks for flagging this. I'm not surprised I missed a few things. Regarding periodic_system and isolated_system those were supposed to be moved out of AtomsBase and then just left in at the very last moment. This is probably how the mistake occured.

I agree with your suggestion about renaming the keyword arguments. (In fact I prefer cell_vectors) but I don't think we will want another breaking change immediately. In the short run somebody should make your script work and make a PR or explain why it shouldn't work and fix the documentation.

If you have time and energy to look into it yourself, I'll do my best to review any PR as quickly as possible.