xraypy/feff85exafs

Feff8L: Open Source theoretical EXAFS fitting standards

feff85exafs's Issues

main Python script is Python2 only

and does not work with Python3... Needs to be fixed in 2018!!

k-grid in feffNNNN.dat

It's hard to say this better than Matt put it in an email to me:

...we should test ... the "write feffNNNN.dat / read feffNNNN.dat"  steps, which 
are kind of weird, really.  chi(k) is broken apart on a strange k-grid, a bunch of 
arrays are down-sampled to single precision, and written out with even less 
precision.  Then we read in those results, interpolate the components of phase/
amplitude as arrays of k and then recompute chi(k) to compare to data.   It might
be fine, but I think the last time this was looked at seriously was 1991.

So ... make the k-grid on which the arrays are saved configurable.

fundamental constants should be updated

There are many file which have ever-so-slightly inaccurate values for fundamental constants:

• bohr is given as 0.529177249 in code, should be 0.52917721067
  src/POT/feffphases.h
src/OPCONSAT/oca_constants.f90
src/XSPH/getedg.f
src/HEADERS/const.h

• ryd is given as 13.605698, should be 13.60569301

src/XSPH/getedg.f
src/HEADERS/const.h

• hbar is given as 1.054572666e-34, should be 1.054571800e-34
  src/DEBYE/sigcl.f (and others here)
  src/OPCONSAT/oca_constants.f90

fixdsp.f --- setting grid

has
xxx(j) = -xx00 + (j-1)*delta

before delta is defined...

EXCH/mpse.f is broken

In file EXCH/mpse.f, mpse.f does not compile with rank mismatches at lines 37 and 57. Variable edens is defined as rank 2 and called as rank 1. To get it to compile, I added ,1 as the second dimension in each case. That just cannot be right.

I suspect that Josh will have to be consulted on this one.

cleanup in wrapper folder

The folders called wrappers/perl and wrappers/python date from a time when I needed something working quickly and so made a SWIG wrapper around the path calculator library. That should go away now.

wrappers/perl-inline-c should be renamed wrappers/perl. And wrappers/python-ctypes should be renamed wrappers/python. The SWIG-based folders should be simply deleted.

Matt, if you could do that for the python, that would be great. I'll do that for the perl.

The bits in src/GENFMT/SConstruct that run SWIG have already been commented out.

Eventually, I will upgrade the testing framework to use the proper python wrappers.

branches

I would like to rewrite history somewhat.

Right now, this repo looks like this:

  o--o--o--A--o-- ... --B  (master)

I would like to change this to

  o--o--o--A     (master)
             \
               o-- ... B (bruce)

where B is the current HEAD and A is bf3a001

After bf3a001, I am the only person who has touched the code. Thus the branch which is at the current HEAD would, then, be my development branch and the master would be fairly close to the form it was in when it was delivered to us by Josh.

To be specific, I want to do the following on a freshly cloned repo:

    git checkout -b bruce
    git checkout master
    git reset --hard bf3a0013bf8f46050965a70ac420a0b97ed0c7f1
    git push origin master -f
    git checkout bruce
    git push origin HEAD

This rewrites history by deleting all commits on master since bf3a001, although that history will be preserved on the "bruce" branch.

It would have been better had I made a branch in the first place, and done my work there. I think that cloning then doing several pull requests was poor practice. What I propose to do will, I think, put this repo in a state that is functionally equivalent to how it would have been had I done the right thing.

But I want another set of eyes to look at what I propose before I do anything.

names of "module binaries" do not convey "feff8l"-ness

Currently (unless I'm mistaken), make install installs

• PREFIX/bin/: ff2x, genfmt, pathfinder, pot, rdinp, xsph

• PREFIX/lib/: json_module.mod, libfeffpath.EXT, libfeffphases.EXT, libjsonfortran.a, libonepath.EXT, libpotph.EXT

• PREFIX/include: feffpath.h, feffphases.h

where EXT='so' for linux, 'dylib' for Mac, 'dll' for Windows.

I see three problems here:

1. there actually isn't a "feff8l" script or executable.
2. there really isn't anything that clearly identifies these as belonging to "feff8l".
3. it's not really clear to me why there are so many "feff8l libraries".

I propose condensing the shared libraries libfeffpath.EXT, libfeffphases.EXT, and libonepath.EXT
to a single libfeff8l.EXT, and changing the name for the installed binary files:

bin/ff2x -> bin/feff8l_ff2chi
bin/genfmt -> bin/feff8l_genfmt
bin/pathfinder -> bin/feff8l_pathfinder
bin/pot -> bin/feff8l_potentials
bin/rdinp -> bin/feff8l_readinp
bin/xsph -> bin/feff8l_phases

We can then also include a bash file (or bat file for Windows) that runs all of these.

unit test coverage

The unit testing framework is certainly helpful, but it does not cover everything in the code. Perhaps the stuff it covers is not super important for the EXAFS mission of this project....

Some examples:

• There are no tests of the OVERLAP card ("card"! I cannot believe we are still using that word in 2014!). Problematic, I suppose, but I don't think that I have used Feff's overlap thingie in the last 20 years.
• There are no tests that include any of the sigma^2 or cumulant stuff. Of course, I think that including such things at the level of Feff makes data analysis more confusing, so I could be comfortable with simply disabling those bits of the code.
• There are no tests that cover things like ION, FOLP, or AFOLP.
• There are no tests that cover non-Hedin-Lvndquist potentials.
• There are not tests to exhaustively cover all the edges.
• A bunch of other crazy stuff -- grids, JUMPRM, INTERSTITIAL, and so on -- is not covered.

POT/broydn.f

rhoold is used at line 88, set at line 102, and preserved by a save statement at line 35.

I presume that this is depending on rhoold getting initialized to 0s on the first call. With save, this is usually defined. But the first time broydn is called, rhoold is ill-defined.

parallel compilation is currently not an option

A sequential compilation is currently hard-wired in PAR/SConstruct. The parallel compilation fails because the mpif.h header file was not included in what the FP gave us. I was able to get the parallel version to compile by copying mpif.h from FDMNES (which I happen to have a copy of...).

Perhaps parallel compilation is not quite a requirement for this project....?

promotion of single to double

One of the things I did in #28 was run the files in src/POT and src/XSPH through some static analyzers, including the venerable ftnchek.

In the files src/POT/frnrm.f and src/POT/sumax.f, ftnchek gives lots of warnings of this sort:

57          jbl = int(2.0+20.0*(blx+8.8))
                                   ^

Warning near line 57 col 33 file sumax.f: promotion may not give desired
precision: dble BLX + real const 8.8

One can explicitly promoting the constants, i.e.,

57          jbl = int(2.0d0+20.0d0*(blx+8.8d0))   

However, doing so results in a few unit test failures. They're not big failures -- that is, I made some arbitrary decision about the level of precision at which I would compare numbers in the unit test suite. These differences are not large compared to that decision. Still, the results end up being a little bit different. I suppose that it's because those two files have a lot of exponentiation, powers, and logarithms in them.

So, what the best thing to do? Don't worry about promotion warnings? Do the explicit promotion and recognize that the answers will be a bit different than the answers using as-delivered feff85exafs? It seems likely that this level of difference will happen on other computers or with other compilers.

build fails on OS X

I get:

scons

gfortran -o src/OPCONSAT/getelement.o -c -O2 -fbacktrace -g -Wall -Wextra -Wno-maybe-uninitialized -J/Users/Newville/Codes/feff85exafs/src/OPCONSAT src/OPCONSAT/getelement.f90
gfortran -o src/OPCONSAT/oca_global_inp.o -c -O2 -fbacktrace -g -Wall -Wextra -Wno-maybe-uninitialized -J/Users/Newville/Codes/feff85exafs/src/OPCONSAT src/OPCONSAT/oca_global_inp.f90
gfortran -o src/OPCONSAT/libfeffloss.os -c -O2 -fbacktrace -g -Wall -Wextra -Wno-maybe-uninitialized -J/Users/Newville/Codes/feff85exafs/src/OPCONSAT -fPIC src/OPCONSAT/libfeffloss.f
../HEADERS/const.h:3:43: Warning: Unused parameter 'alphfs' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:8:21: Warning: Unused parameter 'coni' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:2:55: Warning: Unused parameter 'fa' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:3:27: Warning: Unused parameter 'hart' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:36:20: Warning: Unused parameter 'lamtot' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:45:20: Warning: Unused parameter 'legtot' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:26:18: Warning: Unused parameter 'ltot' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:19:16: Warning: Unused parameter 'lx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:39:18: Warning: Unused parameter 'mtot' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:16:19: Warning: Unused parameter 'nattx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:13:18: Warning: Unused parameter 'natx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:4:20: Warning: Unused parameter 'nclusx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:7:20: Warning: Unused parameter 'nclxtd' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:32:17: Warning: Unused parameter 'nex' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:51:20: Warning: Unused parameter 'nheadx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:48:19: Warning: Unused parameter 'novrx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:29:19: Warning: Unused parameter 'nrptx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:10:18: Warning: Unused parameter 'nspx' declared at (1) [-Wunused-parameter]
../HEADERS/dim.h:39:24: Warning: Unused parameter 'ntot' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:2:51: Warning: Unused parameter 'raddeg' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:2:43: Warning: Unused parameter 'third' declared at (1) [-Wunused-parameter]
../HEADERS/const.h:2:36: Warning: Unused parameter 'zero' declared at (1) [-Wunused-parameter]
gfortran -o src/OPCONSAT/libfeffloss.dylib -dynamiclib src/OPCONSAT/libfeffloss.os src/OPCONSAT/e2emisc.os src/OPCONSAT/eps.os
Undefined symbols for architecture x86_64:
  "___iomod_MOD_closefl", referenced from:
      _write_eps_ in eps.os
  "___iomod_MOD_writearraydata", referenced from:
      _write_eps_ in eps.os
  "_atsym_", referenced from:
      _feffloss_ in libfeffloss.os
  "_chopen_", referenced from:
      _feffloss_ in libfeffloss.os
  "_getelement_", referenced from:
      _write_eps_ in eps.os
  "_terp_", referenced from:
      _intgrl_ in e2emisc.os
      _addeps_ in eps.os
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
scons: *** [src/OPCONSAT/libfeffloss.dylib] Error 1
scons: building terminated because of errors.

not sure why (or why it works on Linux but not OS X).

ints vs long on 64-bit

Vague Error Report Warning:

I think there is a problem with the C wrapper, and probably with other wrappers on 64-bit systems (including Darwin and Linux). I discovered this playing with the Python_ctypes wrapper, and compared the Fortran to C wrapper. Basically, the "ipot" and "iz" arrays in Fortran are "integers", but from C they are called "long". I think they need to be 32-bit integers, and that, as longs, they aren't correct in onepath.f.

I'm not sure what the right solution is....

need a test suite!

compiling with -Wall generates hundreds of warnings

A large fraction have to do with implicit type conversion, so are probably not critical. Still, it would be nice to clean up the code such that gfortran -Wall is not so noisy.

MATH/polint.f compiles with an odd warning

       gfortran -o polint.o -c polint.f
       polint.f:30.72:

                   if (den.eq.0) pause 'failure in polint'                     
                                                                    1
       Warning: Deleted feature: PAUSE statement at (1)

perform static analysis

Matt suggested

 It would be nice to come up with a tree of called functions and 
 dependencies -- I think  there are quite a few completely unused 
 functions, and lots of commented out code blocks.   These should
 all be removed - they exist in the initial git commit, so are not
 lost to history, but there's no reason to have to look at that.

need to get rid of ".bin" extensions

Many of the temporary files from feff use the ".bin" extension to denote the "binary-data-ness" of the contents. This should be replaced.

First, the data shouldn't be raw fortran binary, as its not portable. The PAD library might still be useful, or perhaps something like hdf5 should be used.

In any event, the ".bin" extension is a pain in and of itself because it gives a stupid false-positive for many spam filters. Though mostly serving to underscore the poor quality of spam filters that rely on advertised file extensions instead of actual file contents, the ".bin" extension is generally perceived as poison and should be replaced.

small error in calculation of mag[feff]

I see that the value of mag[feff] for any path I have checked (not many) is currently significantly different from the baseline calculation in the test suite, but only at k=0. In practice, this will have no impact on data analysis (which typically does not include k=0 in the fit and which typically uses k-weighting). However, this is a bug.

Compilation with -finit-local-zero fails

Using gfortran's -finit-local-zero flag seems like a good idea. Read about it here.

However, FMS/fmstot.f fails with this error:

gfortran -o FMS/fmstot.o -c -O3 -ffree-line-length-none -finit-local-zero FMS/fmstot.f
FMS/fmstot.f:45.26:

 complex*16 dck, bmat                                              
                    1

Error: Automatic array 'bmat' at (1) cannot have an initializer
FMS/fmstot.f:43.34:

 complex gtr(ipmin:ipmax,nex),gtrloc(ipmin:ipmax,nex) !KJ I added i
                             1

Error: Automatic array 'gtr' at (1) cannot have an initializer
FMS/fmstot.f:43.58:

 complex gtr(ipmin:ipmax,nex),gtrloc(ipmin:ipmax,nex) !KJ I added i
                                                     1

Error: Automatic array 'gtrloc' at (1) cannot have an initializer
scons: *** [FMS/fmstot.o] Error 1