cortner / SKTB.jl

Other
9 stars 4 forks source link

Currently broken; assume due to changes in JuLIP and ASE #20

Closed jarvist closed 6 years ago

jarvist commented 6 years ago

Currently the package is a little broken - I assume due to your ASE and JuLIP packages changing underneath it.

In fixing, I got as far as:

diff --git a/src/calculators.jl b/src/calculators.jl
index 22af86d..51a9931 100644
--- a/src/calculators.jl
+++ b/src/calculators.jl
@@ -1,5 +1,5 @@

-using JuLIP: set_transient!, get_transient
+using ASE: set_transient!, get_transient, has_transient

 # this file implements the standard spectral decomposition
diff --git a/test/runtests.jl b/test/runtests.jl
index a9c52d8..f8abff1 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -1,5 +1,6 @@
 using TightBinding
-using JuLIP, JuLIP.ASE
+using JuLIP 
+using ASE
 using JuLIP.Testing
 using Base.Test

But still crunch to a halt with:

> julia runtests.jl 
============================================
    TightBinding Tests  
============================================
Testing TB Toy Model
length(at) = 24
-------------------------------------------
Testing ToyTBModel: 
length(at) = 24
check that hamiltonian evaluates ... ToyModel: Error During Test
  Got an exception of type ErrorException outside of a @test
  type NeighbourList has no field levels
  Stacktrace:
   [1] TightBinding.SparseSKH(::TightBinding.ToyHamiltonian{JuLIP.Potentials.ProdPot{JuLIP.Potentials.AnalyticFunction{JuLIP.Potentials.##27#30{Float64,Float64,Float64},JuLIP.Potentials.##28#31{Float64,Float64,Float64},JuLIP.Potentials.##29#32{Float64,Float64,Float64}},JuLIP.Potentials.SWCutoff}}, ::ASE.ASEAtoms) at /home/jarvist/.julia/v0.6/TightBinding/src/matrixassembly.jl:180
   [2] evaluate(::TightBinding.ToyHamiltonian{JuLIP.Potentials.ProdPot{JuLIP.Potentials.AnalyticFunction{JuLIP.Potentials.##27#30{Float64,Float64,Float64},JuLIP.Potentials.##28#31{Float64,Float64,Float64},JuLIP.Potentials.##29#32{Float64,Float64,Float64}},JuLIP.Potentials.SWCutoff}}, ::ASE.ASEAtoms) at /home/jarvist/.julia/v0.6/TightBinding/src/types.jl:48
   [3] macro expansion at /home/jarvist/.julia/v0.6/TightBinding/test/testtoymodel.jl:20 [inlined]
   [4] macro expansion at ./test.jl:860 [inlined]
   [5] anonymous at ./<missing>:?
   [6] include_from_node1(::String) at ./loading.jl:576
   [7] include(::String) at ./sysimg.jl:14
   [8] include_from_node1(::String) at ./loading.jl:576
   [9] include(::String) at ./sysimg.jl:14
   [10] process_options(::Base.JLOptions) at ./client.jl:305
   [11] _start() at ./client.jl:371

If you dig down, you can see that it is breaking on this, from matrixassembly.jl:180

julia> nlist = neighbourlist(at, 1.2)
ASE.MatSciPy.NeighbourList(1.2, Int32[], Int32[], Float64[], StaticArrays.SArray{Tuple{3},Float64,1,3}[], StaticArrays.SArray{Tuple{3},Int32,1,3}[], (Int32[], Int32[], Float64[], , ), 24)

julia> length(nlist)
ERROR: type NeighbourList has no field levels
Stacktrace:
 [1] length(::ASE.MatSciPy.NeighbourList) at /home/jarvist/.julia/v0.6/AMG/src/multilevel.jl:21

At this point I'm pretty lost! I guess the ASE NeighbourList object is arriving not as the expected MultiLevel type, and therefore doesn't have the necessary fields...

cortner commented 6 years ago

This package is more experimental than the others, hence not registered. I therefore didn’t prioritise it. But it should work in principle. I’ll do some tests.

cortner commented 6 years ago

Are you actually interested in using this (potentially) or are you just trying to figure out what is available? Just to be clear: I would under no circumstances use this package for an actual materials modelling publication. I use it to develop tweaks and modifications to TB models that I can't do (easily) in production software. Maybe if somebody more knowledgeable can invest some time it could actually become more reliable from the physics/chemistry perspective.

jarvist commented 6 years ago

Potentially both! I'm getting back into Tight Binding method development; particularly Polarisable Ion TB with the Hairy Probe open-boundary method - applied to computational corrosion. The production codes (https://questaal.org/docs/code/tbe/ ; https://en.wikipedia.org/wiki/PLATO_(computational_chemistry) ) are a little inflexible for model development, so having a Julia implementation to develop tweaks and play with fitting methods would be pretty useful.

cortner commented 6 years ago

needless to say, any contributions are extremely welcome! Even just letting me know where there are shortcomings in your view would already be very helpful!

I'll try to fix this asap.

cortner commented 6 years ago

sorry for the delay - I got bogged down preparing a new talk. I'll get on this now.

cortner commented 6 years ago

Everything should be fine now. (It was actually just a matter of merging the right branch, I just never got around to making sure everything is still working with the latest updates.)

I'm assuming you installed JuLIP just via Pkg.add, but if you ever checkout out the master branches on any of my other packages, then please

Pkg.free("JuLIP")
Pkg.free("NeighbourLists")

and then

Pkg.checkout("TightBinding")
Pkg.test("TightBinding")

If this works fine, then please close issue, otherwise just let me know. Anything else not working as expected just let me know or just make a PR. (I try to react quickly)

cortner commented 6 years ago

Just FYI - the main thing that needs to be (triple-)checked in this version of TightBinding is the implementation of the spd Slater-Koster matrices. I'm somewhat confident that the sp Slater-Koster implementation is ok.

jarvist commented 6 years ago

That's great Christoph! All tests pass.

spd is the part of the periodic table I now inhabit, so I'll check it against the tbe codes once I get our model Hamiltonian implemented.

Many thanks! Reading the code, there's some really nice features you've implemented.