stand alone library for photoelectron phase shifts

[bruceravel]

This is clearly an enormous PR, but in fact it changes rather little in the existing code base. The vast majority of the changes involve the addition of the Fortran and C files in src/POT which implement the interface to the stand-alone library. There are a few other changes in the directory to accommodate the stand-alone library, but all prior behavior of the package remains in place. Specifically the pot stand-alone executable is still built and is still intended to be run after the rdinp stand-alone.

This PR implements the library itself as well as examples and wrappers in Fortran, C, and Perl under the wrappers directory. All of them work as expected, producing output consistent with the content in tests/Copper/baseline/. The C wrapper run valgrind-clean. I have not taken a stab at a python wrapper.

The I/O model remains a little primitive. The notion is that a UI would collect the sort of information that gets passed to the library (atoms list, potentials list, and so on). For now, the rdinp stand-alone writes a file called libpotph.json which is basically a JSON serialization of the struct from feffphases.c. There is a method in feffphases.c for reading that JSON file using nxjson.

The I/O structure of the Fortran entry point and C wrapper are discussed in README files in the respective wrappers directories. Those READMEs are part of this PR and constitute an initial stab at documentation.

With this and the mostly complete work on feffpath, I have basically everything needed to ditch feff6 and system calls in Artemis. That is, Artemis can handle the I/O issues, call the feffphases library, run its own pathfinder, then call the feffpath library many times. Neat-o!

I don't think this work on the feffphases library is quite finished, but it is far enough along that I wanted to move it into the main branch. I will leave it here for a day or so before merging, unless someone beats me to it. Once it is merged into the main branch, I will open some issues requiring discussion or action by someone else (Matt, I mean action by Matt :smile:).

[newville]

OK, that seems like a lot of work! Sorry if this seems grumpy, but a PR this large is really hard to review or really even comprehend. It seems the basic point is that there's now a standalone POT module -- is that right? But it's only standalone in the sense of needing to be run after an input translation layer (rdinp) -- is that right?

I think it might be helpful to discuss the basic design, or discuss it again now that you're this far along.

[newville]

... but also: Yes, definitely. I'll try to comprehend this enough to work on the Python wrapper.

[bruceravel]

I know that you have complained about large PRs before, and I am trying very hard not to be peevish in this comment.

The stand alone Fortran library combines the functionality of POT and XSPH from Feff. It takes information of the sort one finds in feff.inp in the form of a long argument list and runs through the calculation of the potentials (possibly with self-consistency) and then computes & writes the phase.pad file.

In wrappers/fortran there is a markdown file that explains the argument list and an example of using the library. I had hoped that would be adequate explanation.

Nothing here tries to solve the user interface problem of getting the information contained in feff.inp into the library. (I think that should be a python/perl/whatever level solution.) The library expects a long argument list. I made rdinp write out a JSON serialization of argument list as a stop-gap measure so I could test my work.

In all ways, the design is identical to the design of the path calculator from a few months back. This is the design concept you yourself called for -- a Fortran functions with long argument lists. That can be discussed, but I thought we had already done that.

:hourglass_flowing_sand: :hourglass_flowing_sand: :hourglass_flowing_sand:

Hmmmm .... as I was writing this, you merged the PR. I am not certain how to have a discussion when discussion time is truncated shorter than the amount of time I need to respond. I was going to consider rejecting the PR and breaking it into smaller pieces. Guess that's not needed.

As far as a python wrapper goes -- it should be extremely similar to the wrapper around the path calculator library. Again, I hope that the readme files in wrappers/fortran and wrappers/C will be helpful.

[newville]

Hi,

Sorry, kind of a busy day.

On Tue, Jul 7, 2015 at 2:13 PM, Bruce Ravel notifications@github.com wrote:

I know that you have complained about large PRs before, and I am trying very hard not to be peevish in this comment.

It's really mostly that it's a bit hard to comprehend 30 or more commits as one thing, especially when some are about the build system, some about the wrapper, some refactoring, and so on. Like, if we take the view that this is all sort of alpha-y anyway, it's all OK -- we started with such a poor API and build system that they need to be built from scratch, and the testing is really only on output results (the only thing we really can test), not really unit-tests of calling individual routines (at least not yet). So, in that sense, it's all building new code,

But having more frequent, smaller PRs isolated to different components would be way easier to try to understand.

The stand alone Fortran library combines the functionality of POT and XSPH from Feff. It takes information of the sort one finds in feff.inp in the form of a long argument list and runs through the calculation of the potentials (possibly with self-consistency) and then computes & writes the phase.pad file.

Yeah, that's awesome!

In wrappers/fortran there is a markdown file that explains the argument list and an example of using the library. I had hoped that would be adequate explanation.

OK. I just saw this. And, yes, in general it's all fine with me.

Nothing here tries to solve the user interface problem of getting the information contained in feff.inp into the library. (I think that should be a python/perl/whatever level solution.) The library expects a long argument list. I made rdinp write out a JSON serialization of argument list as a stop-gap measure so I could test my work.

OK, that makes sense. I just couldn't tell whether the JSON parts of the PR were primary goals, but it seems they're intended as intermediate steps.

In all ways, the design is identical to the design of the path calculator from a few months back. This is the design concept you yourself called for -- a Fortran functions with long argument lists. That can be discussed, but I thought we had already done that.

[image: :hourglass_flowing_sand:] [image: :hourglass_flowing_sand:] [image: :hourglass_flowing_sand:]

Hmmmm .... as I was writing this, you merged the PR. I am not certain how to have a discussion when discussion time is truncated shorter than the amount of time I need to respond. I was going to consider rejecting the PR and breaking it into smaller pieces. Guess that's not needed.

I think it would be fine to just to commit to master as you go. It would be simpler and easier to follow. It's not like we're really concerned with breaking API or even failing tests yet.

As far as a python wrapper goes -- it should be extremely similar to the wrapper around the path calculator library. Again, I hope that the readme files in wrappers/fortran and wrappers/C will be helpful.

Yep, I'll try to get to this.

--Matt

[bruceravel]

On 07/07/2015 09:30 PM, Matt Newville wrote:

I think it would be fine to just to commit to master as you go. It would be simpler and easier to follow. It's not like we're really concerned with breaking API or even failing tests yet.

Very well. I'll do so from now on. B

Bruce Ravel ------------------------------------ bravel@bnl.gov

National Institute of Standards and Technology Synchrotron Science Group at NSLS-II Building 535A Upton NY, 11973

Homepage: http://bruceravel.github.io/home/ Software: https://github.com/bruceravel Demeter: http://bruceravel.github.io/demeter/