Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AtomsBaseTesting.test_approx_eq behaviour for identical systems with different units #128

Closed
Alexsp32 opened this issue Oct 28, 2024 · 1 comment · Fixed by #129
Closed
Labels
bug Something isn't working

Comments

@Alexsp32
Copy link

AtomsBaseTesting.test_approx_eq does not consider two identical systems using different units (e.g. Bohr and Å) as equal.
This behaviour is different to previous versions and seems to be related to the addition of cell tests in #121.

Example:

using AtomsBase
using AtomsBaseTesting
using Unitful, UnitfulAtomic
using Test

box = 10.26 / 2 * [[0, 0, 1], [1, 0, 1], [1, 1, 0]]u"bohr"
box_A = [[uconvert.(u"Å", i[j]) for j in 1:3] for i in box]
silicon = AtomsBase.periodic_system([:Si =>  ones(3)/8,
                           :Si => -ones(3)/8],
                           box, fractional=true)
silicon_A = AtomsBase.periodic_system([:Si =>  ones(3)/8,
                           :Si => -ones(3)/8],
                           box_A, fractional=true)

AtomsBaseTesting.test_approx_eq(silicon, silicon_A)

Fails with:

  Expression: typeof(cell(s)) == typeof(cell(t))
   Evaluated: AtomsBase.PeriodicCell{3, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(Å,), 𝐋, nothing}}} == AtomsBase.PeriodicCell{3, Unitful.Quantity{Float64, 𝐋, Unitful.FreeUnits{(a₀,), 𝐋, nothing}}}
@mfherbst
Copy link
Member

Indeed, that makes sense. Thanks for flagging.

@mfherbst mfherbst added the bug Something isn't working label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants