Unusual Keys for `***_data, ***_info, ***_transient`

[cortner]

The is a bug that arises in Tightbinding.jl

using JuLIP
k = rand(JVecF)
print("First Call to `bulk`... ")
at1 = bulk("Si")
set_transient!(at1, (:a, k), rand())
println("OK!")
println("Second Call to `bulk` ... " )
at2 = bulk("Si")

Somehow, magically, the call set_transient!(at1, (:a, k), rand()) messes up the second Python call. Moreover, it is the specific choice of key k, if instead we use

set_transient!(at1, (:a, k...), rand())

then everything is fine.

[cortner]

just for curiosity: using k itself as key also crashes.

[cortner]

So it turns out that uncommenting the checks whether a key exists in another array

function set_transient!(a::ASEAtoms, name, value, max_change=0.0)
   # if has_array(a, name) || has_info(a, name)
   #    error("""cannot set_transient!(..., $(name), ...)" since this key already
   #             exists in either `arrays` or `info`""")
   # end
   a.transient[name] = TransientData(max_change, 0.0, value)
   return a
end

resolves the problem. (the array and info dictionaries live in Python!) My best guess is that the vector k is converted into a Python list, which causes some havoc. Why the crash occurs on the second call to bulk is beyond me though.

In response I have temporarily removed get_data, set_data!, has_data and have removed all similar checks in get_info and get_array. But this is not a long-term solution . . .

[cortner]

@jameskermode sorry to ping you, but I am wondering whether this is related to the unexplained bugs we've seen occasionally, including #72 which Gideon dug up?

[cortner]

@gideonsimpson I just ran your code without problems. But I don't have the same environment as you, or maybe I didn't fully reconstruct the setup where I managed to make it crash. So would you be wiling to pull the latest master of JuLIP and try to run your code again? From the Julia REPL just call

Pkg.checkout("JuLIP.jl")

[cortner]

If this resolves Gideon’s Problem then we could switch to Python 3!

[jameskermode]

You can't use a Python list as a dictionary key as lists are mutable - this is what the original error message in https://github.com/cortner/TightBinding.jl/issues/18 refers to. Instead we should make a tuple from the list before trying to use it as a dictionary key.

[jameskermode]

Not related to #72 as far as I can tell, that was a subtle memory overwriting bug in PyCall or Julia. This is a simple Python usage error by us that we can fix.

[jameskermode]

Python code for list to tuple conversion is simply key = tuple(orig_key).

[cortner]

Are you certain? Note that the error is thrown on construction of a new atoms object!

[jameskermode]

I haven't tested, but that explanation would certainly fit the stack trace you posted in https://github.com/cortner/TightBinding.jl/issues/18

[cortner]

What about the code in my first post kn this thread?

[jameskermode]

Let me test, I agree it appears strange.

[jameskermode]

I can't reproduce crash using code from this thread with latest JuLIP master plus commit 9d14458b2ff reverted, julia v0.6.2, Python 2.7.13 - second call to bulk() completes fine.

[cortner]

Latest master fixes the crash - need to go 1 or two commits back. I can instruct later when I am in my laptop

[jameskermode]

I think I reverted the relevant commit. I still haven't reproduced, but perhaps relatedly it looks like there is some inconsistency between whether keys are converted to strings or passed as is on to Python, e.g. has_array() checks for a key name, while get_array() and set_array() use string(name).

In general, using float arrays as dictionary keys feels like a bad design decision as rounding could lead to mismatches. Would be better to use integers.

[cortner]

Ok - fair point. Though in our use case this should not have been a problem. I’ll wait to see whether this fixes Gideon’s problem though.

For reference which versions of python and ASE are you using?

[jameskermode]

Python 2.7.13 (anaconda), ASE 3.15.1b1

[cortner]

• Python 2.7.13 :: Anaconda custom (x86_64)
• ASE version 3.14.1

I will try to see what happens if I update it.

[gideonsimpson]

For my Anaconda Python 3 installation, with respect to @cortner's example above, I got the following output:

julia> using JuLIP

julia> k = rand(JVecF)
3-element SVector{3,Float64}:
 0.817104
 0.491234
 0.769904

julia> print("First Call to `bulk`... ")
First Call to `bulk`... 
julia> at1 = bulk("Si")
JuLIP.ASE.ASEAtoms(PyObject Atoms(symbols='Si2', pbc=True, cell=[[0.0, 2.715, 2.715], [2.715, 0.0, 2.715], [2.715, 2.715, 0.0]]), JuLIP.NullCalculator(), JuLIP.NullConstraint(), Dict{Any,JuLIP.ASE.TransientData}())

julia> set_transient!(at1, (:a, k), rand())
JuLIP.ASE.ASEAtoms(PyObject Atoms(symbols='Si2', pbc=True, cell=[[0.0, 2.715, 2.715], [2.715, 0.0, 2.715], [2.715, 2.715, 0.0]]), JuLIP.NullCalculator(), JuLIP.NullConstraint(), Dict{Any,JuLIP.ASE.TransientData}(Pair{Any,JuLIP.ASE.TransientData}((:a, [0.817104, 0.491234, 0.769904]), JuLIP.ASE.TransientData(0.0, 0.0, 0.594875))))

julia> println("OK!")
OK!

julia> println("Second Call to `bulk` ... " )
Second Call to `bulk` ... 

julia> at2 = bulk("Si")
JuLIP.ASE.ASEAtoms(PyObject Atoms(symbols='Si2', pbc=True, cell=[[0.0, 2.715, 2.715], [2.715, 0.0, 2.715], [2.715, 2.715, 0.0]]), JuLIP.NullCalculator(), JuLIP.NullConstraint(), Dict{Any,JuLIP.ASE.TransientData}())

With regard to my own issue, in #72 I think you had implemented some workaround previously so that it no longer crashed. But it also no longer crashses with whatever is currently implemented. It also works fine with the current checkout.

[cortner]

After upgrading Anaconda and ASE to 3.15 I no longer crash either.

[cortner]

Thank you Gideon for testing though.

All this leaves me with a bad feeling that I want to rely less on Python/ASE I the future. Will probably rewrite ASEAtoms very soon now.

I'm closing this but am leaving a tag in the git repository to remember where this happened. I will leave the ***_data deleted for now and want to think about it for a while.