search

tfrederiksen / inelastica

Python package for eigenchannels, vibrations and inelastic electron transport based on SIESTA/TranSIESTA DFT
https://tfrederiksen.github.io/inelastica
GNU Lesser General Public License v3.0
33 stars 16 forks source link

Bug in findLattice methode at Symmetry module #26

Closed brandimarte closed 6 years ago

brandimarte commented 6 years ago

Depending on how the atoms are defined with respect to the cell origin, one can get an error when trying to find the lattice parameters.

=======================================================================
INELASTICA VERSION : v1.3.0-69-g49b4ed2
RUNNING BANDSTRUCTURES : Fri Mar  2 22:53:35 2018

OPTIONS :
     AtomicMass --> []
     DestDir --> aver
     FCwildcard --> ./FC*
     Logfile --> Bandstructures.log
     kfile --> None
     mesh --> [0,0,0]
     nbands --> None
     onlySdir --> ./OSrun
     onlyTSdir --> .
     qfile --> None
     radius --> 0.0
     sorting --> False
     steps --> 100
=======================================================================
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/RUN.fdf
io.siesta.ReadFDFLines: Reading /home/pedro/machines/oberon/pedro/doc/Ge2probe/surfGe/relax_big/results_new/STRUCT.fdf
io.siesta.ReadXVFile: Reading ./surfGe.XV
Structures in fdf and XV are different! It has been relaxed?

Performing symmetry analysis
Traceback (most recent call last):
  File "/home/pedro/local/opt/inelastica/latest/python2.7/bin/Bandstructures", line 12, in <module>
    Module.main(options)
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/SupercellPhonons.py", line 487, in main
    SCDM = Supercell_DynamicalMatrix(fdf, TSrun)
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/SupercellPhonons.py", line 115, in __init__
    self.CheckSymmetries(TSrun=TSrun)
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/SupercellPhonons.py", line 124, in CheckSymmetries
    Sym.setupGeom(self.geom.pbc, self.geom.snr, self.geom.anr, self.geom.xyz, onlyLatticeSym=True)
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/Symmetry.py", line 81, in setupGeom
    self.findSymmetry(onlyLatticeSym)
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/Symmetry.py", line 96, in findSymmetry
    self.findLattice()
  File "/home/pedro/local/opt/inelastica/latest/python2.7/lib/python2.7/site-packages/Inelastica/Symmetry.py", line 538, in findLattice
    a3 = possible[i3]
IndexError: index 115 is out of bounds for axis 0 with size 115
brandimarte commented 6 years ago

I tried to find the culprit and look's like the problem is in the myUnique call. I think it was suppose to sort the coordinates first in x, then in y, then z, but this does not happen, but I'm not sure if this was really the intention there, was it? @mpn2?

Adding the following lines just after myUnique call seems to solve the problem:

        # sort along the x component
        possible.view('f8,f8,f8').sort(order=['f0'], axis=0)
mpn2 commented 6 years ago

If you attach an example geometry which gives the error I can have a look. I don't remember too much ...

I think the stress test still sometimes fails, i.e., try python Symmetry.py (outside the Inelastica directory) and it randomly used to fail after a few hundred tries ...

brandimarte commented 6 years ago

Hi @mpn2 !

Thanks, it would be great if you could have a look at it. My guess is that in findLattice method when you perform the loop

while i1<NP-2 and not done:

you are somehow expecting that the set should be ordered in the first dimension x, that's why I included this sort in the myUnique call, which sort the set by distance instead. In the attached file there is an example where the Bandstructures script fails.

Best!

slabGe.tar.gz

tfrederiksen commented 6 years ago

I updated the module such that the torture test is executed with a script like this:

import Inelastica.Symmetry as SYM
SYM.test()

But I can confirm that this test still sometimes fails (in a first run after 5 steps and in the second after 435):

Running test no. 435
Symmetry: Lattice structure
4 atoms in the basis
a1 = (-1.450211,-0.772012,-1.816834), N1=5
a2 = (1.375195,-0.902704,-1.814929), N2=2
a3 = (-0.059729,-1.282632,0.592693), N3=9

b1 = (-0.357864,-0.088333,-0.227223)
b2 = (0.348487,-0.121006,-0.226747)
b3 = (-0.029865,-0.641317,0.296347)
Shifting coordinates by :  [ 0.01261967  0.61510764  0.7335724 ]
Symmetry: ERROR! Inconsistent number of basis atoms. Probably bug in program.
Symmetry error
brandimarte commented 6 years ago

As @tfrederiksen point out in the comment on 9f67bcc, including the sorting over x do not fix the issue, since running the torture test it fails right way.

Nevertheless, this error in the torture test above might be pointing to a different issue, because here looks like the error happens after it finds the lattice.

Best!

tfrederiksen commented 6 years ago

Hi @mpn2 and @brandimarte,

Just to add some details: To test Pedro's slabGe example try this:

Bandstructures --TSdir=<slabGe_dir> <dest_dir>

This generates IndexError: index 119 is out of bounds for axis 0 with size 119. It appears to me that this is may be a bug in the function increase(i1,i2,i3,NP,incindx) (it should never return an index equal to the array length NP). In the example we have NP=119.

brandimarte commented 6 years ago

Indeed @tfrederiksen! For instance if it happens that i2==i3==NP-1 and incindx==2 than i3 will be assign to NP+1.

brandimarte commented 6 years ago

Hi again!

I think I started to understand a bit the idea of the algorithm at findLattice. The sort in myUnique call is done by distance since one wants to find the "smaller" lattice vectors, so it starts with the smaller set possible. In the example slabGe above it turns out that the third lattice vector a3 is the largest possible which might explain why it goes out of bounds. Maybe changing the last lines of increase function as:

            i2 = min(max(i1+1, i2),NP-1)
            i3 = min(max(i2+1, i3),NP-1)
            #if incindx<=1: print i1, i2, i3
            return i1, i2, i3

But still in this case the torture test fails. I think it would require a careful look at checkLattice though to understand how incindx is defined. Maybe one should get first all a1,a2,a3 valid candidates and then choose those that gives the smaller volume.

mpn2 commented 6 years ago

I have no idea of how git works yet ... this fixes the increase function so that it works as designed. The problem was that if i3>=NP -> i3=i2+2; i2=i2+1 ... now if i3 was NP-1 and i2 NP-2, i3 would increase too much. That is now fixed. Can someone add this. The fix above from brandimarte could work but I'm not a 100% sure.

    def increase(i1, i2, i3, NP, incindx):
        # Keep track of indices
        if incindx<2:
            i3=0
        if incindx<1:
            i2=0
        if incindx==2:
            i3=(i3+1)%NP
            if i3==0:
                i3=(i2+2)%NP
                incindx=1
        if incindx==1:
            i2=(i2+1)%(NP-1)
            if i2==0:
                i2=(i1+2)%(NP-1)
                incindx=0
        if incindx==0: i1+=1
        i2 = max(i1+1, i2)
        i3 = max(i2+1, i3)
        #if incindx<=1: print i1, i2, i3
        return i1, i2, i3
mpn2 commented 6 years ago

About the slabGa exampe: There is no symmetry above the unitcell for this example (at the default accuracy of the Symmetry function=0.0001 Å). I think there should always be the option of setting the accuracy for the symmetry accuracy because of the approximate relaxed geometries that Siesta gives.

mpn2 commented 6 years ago

I'll look at the torture test ...

tfrederiksen commented 6 years ago

Hi @mpn2 ,

We could commit it, yes, but why not try out these simple steps yourself to get started?

git clone git@github.com:tfrederiksen/inelastica.git
cd inelastica/Inelastica
# edit Symmetry.py
git add Symmetry.py
git commit -m "bug: fixed increase(...) function"
git push
brandimarte commented 6 years ago

Great @mpn2 !

My idea above was only a "maybe". :-) I'll include the fix, but here are the steps I will do in my local machine to give an idea how is the workflow with git:

cd /path/to/inelastica
git fetch
git pull
# edit the code
git add Inelastica/Symmetry.py
git commit -m "Include fix to #26 by Magnus"
git push

Best!

brandimarte commented 6 years ago

Yep!! @mpn2 go ahead and commit it! :-)

mpn2 commented 6 years ago

I assume this is fixed now. The failure of the tourture test is a separate issue which I will fix if someone actually finds a real example where the issue appears.