Support XSPEC 12.10.0 (fix #436)

[DougBurke]

Release Note

Sherpa now supports XSPEC 12.10.0 (currently limited to CIAO 4.11)

Details

This adds the following XSPEC models (all additive) when built against
XSPEC 12.10.0:

XSbrnei, XSbvrnei, XSbvvrnei, XSgrbcomp, XSjet, XSssa, XSzcutoffpl

The following models now use C/C++ rather than the FORTRAN interface,
so may result in numerical differences (when using XSPEC 12.10.0 or
later):

XSc6mekl, XSc6pmekl, XSc6pvmkl, XSc6vmekl, XSnsmax, XSnsmaxg, XSnsx,
XSacisabs, XSswind1, XSzxipcf

The minimum value of the Gamma parameter of the XSoptxagnf model has been
increased to 1.05 - it was 0.5 - to match the setting in the XSPEC 12.10.0 model.dat
file (this change happens whatever version of XSPEC is used).

The low-level interface to the gsmooth and lsmooth convolution models
has been updated to use the updated function names for XSPEC 12.10.0
or later.

The documentation has been updated to note explicit support for XSPEC
12.10.0, and the old advice of how to build against the CIAO build of
XSPEC has been removed as it was not founf to be that useful.

The tests have been updated to match the 12.10.0 behavior; this is
primarily increasing tolerances for the models which have changed from
FORTRAN to C/C++ interfaces. The tolerances were adjusted until they
passed on a Linux machine; they were not checked to make sure they
still made sense.

Notes

This has only been tested on Linux and it would not be surprising if the
build fails on OS-X, since the steps needed to call the xs_getVersion
function from the XSPEC library are much-more involved.

An alternative approach to the config shenanighans seen here would be to switch to an auto-generated module. That could be done after accepting this PR to get initial support for XSPEC 12.10.0, or maybe it's worth doing now?

PR Walk Through

README.md

The README.md file has been updated, adding a section for XSPEC 12.10.0 and later, and retaining the existing section, labelling it as XSPEC 12.9.x. The major difference is the need to include hdsp_2.9 in the xspec_libraries setting with 12.10.0. The discussion of how to build against CIAO's version of XSPEC has been removed as it was not that useful (you needed to have the same setup as used to build CIAO, and that is quite hard to achieve).

Note that building XSPEC 12.10.0 requires hdsp_2.9 to the link line: the documentation suggests adding it to the xspec_libraries setting, but an alternative would be to add a separate config option (as we do for cfitsio/CCfits/wcslib).

helpers/xspec_config.py

I separated out the code to try and find the XSPEC version by dynamically loading the needed library (or libraries), as this is quite different in 12.10.0 to 12.9.x. I have not tested this on macOS, or against a models-only build (I have been testing against a full XSPEC build), so I expect this section may need significant changes.

An alternative to this would be to include a small C (or even fortran) file that is just a wrapper around the xs_getVersion routine, and use distutils (and the configuration settings) to compile it to an executable which can be run to find out the version number. I have not explored this approach.

sherpa/astro/xspec/init.py

Mainly just adding in the new models. Some tweaks to existing documentation (e.g. adding in "see also" and fixing a problem with a see also to an invalid name).

The minimum permitted value for a parameter in XSoptxagnf has changed from 0.5 to 1.05 in XSPEC 12.10.0; I have changed this for all versions as it is likely a valid change even for older models, and we don't have an easy way to change parameter settings with different versions.

sherpa/astro/xspec/src/_xspec.cc

Two changes:

a) adding new symbols for the new models

b) remove un-needed definitions since we can just get them from the XSPEC include file (unfortunately I don't think this is possible for the F77 API calls if you just have the installed, and not build, location of XSPEC available).

sherpa/astro/tests/test_astro.py

The model code has changed, so the output is different, hence the best-fit parameters are different. I have just tweaked the tolerance to let the tests pass with 12.10.0. As indicated in the comments, either I mis-understood the old test code - which I have replaced with numpy.testing.assert_allclose - or there is a potential issue with the test having little useful power (probably the former).

I have not looked at the data for this test, but I wonder how useful it is (with such a large kT).

sherpa/stats/tests/test_stats.py

Again I just tweaked the tolerance until the fit passed. The nH value is the problem here, and the best-fit values are quite large (~3500).

edit I am now seeing this test (test_stats.py) fail on a local installation with XSPEC 12.9.1p and without this PR installed, so I wonder if this is actually due to some environmental change (e.g. NumPy) rather than XSPEC. Hmm, downgrading to NumPy 1.12 from 1.14 didn't change it, so maybe it's a compiler version? Will have to investigate further.

[DougBurke]

The fact that the Travis tests pass is obviously good, but it only tests against XSPEC versions 12.9.0 and 12.9.1.

@olaurino - does the fact the XSPEC Travis builds pass confirm that the XSPEC module is being built and tested? I think it does, but am not sure (I think you added code a long-time back for the fits and xspec support that ensures this). The run times for the xspec tests are longer than the others, which suggests the XSPEC tests are being run, and I could look in the logs to find out, but looking at a green tick mark is so-much easier.

[DougBurke]

So I don't forget: the build configuration code to find out the XSPEC version number is now ugly. I wonder if instead of trying to load the code from within Python, we ship a small C program that prints the result of xs_getVersion which we could compile and run during the setup phase. It would still be messy, since we need to send in the configuration options to build the executable, but it may be a bit-more robust. Without being able to test on an OS-X machine I don't know which way is preferable.

[olaurino]

does the fact the XSPEC Travis builds pass confirm that the XSPEC module is being built and tested?

See e.g. https://travis-ci.org/sherpa/sherpa/jobs/381320239#L1351

There is indeed special code that requires the smoke test to be able to import xspec, which should also mean that the regression tests are run as well. When I work on the xspec interface I try and check that tests run properly in the Travis jobs, because there is no limit to the things that might not go as expected.

[DougBurke]

When putting this PR together I noted that a couple of parameter names aren't valid Python - e.g. XSPEC has log(xxxx). I've checked with the XSPEC team and they say they'll replace them with log_xxx. I'll adjust this PR when I get time to reflect this change.

For reference:

% % grep \( heasoft-6.23/spectral/manager/model.dat
log(A)   " "   5     -8     -8       8        8       -0.01
log(mdot) "logL/LEdd"  -1. -5. -5.   2.       2.         1e-2
log(xi)  " "  1.0  1.0     1.0     6.0       6.0         0.02
log(xi)  " "  1.0  1.0     1.0     6.0       6.0         0.02

and these come from grbcomp, jet, and rfxconv/xilconv. The first two are new to 12.10.0, the last two are new in 12.9.1 but are convolution-style models, so we don't have Python classes set up for them.

[DougBurke]

The adjustments for the "non-Pythonic" parameter names can be found in changeset 9ad6639

[olaurino]

This PR is blocked because I can't have a proper xspec 12.10 build. I contacted the XSPEC developers because of what looks like an issue in their new build system, but haven't heard back from them.

[DougBurke]

I've rebased this, and updated some documentation, but added it as pr #497 in case it messes with anything @olaurino has done

[olaurino]

We are working to get this in CIAO 4.11, but it's unlikely this will get in standalone 4.10.1

[olaurino]

rebasing to resolve conflicts. Was 5841b24. I am also pushing the code that introduces a configuration option for directly setting the XSPEC version, in order to support XSPEC 12.10.0. At this point the priority is to make it work in CIAO 4.11.

[olaurino]

The failure at this point (https://travis-ci.org/sherpa/sherpa/jobs/441669790) is due to a quirk in the XSPEC build process that does not produce all the necessary libraries. I will have to re-build the xspec conda binary to make progress, following workarounds the XSPEC team suggested (and which are already part of the new OTS in CIAO). However at this point the priority is to make this work in CIAO, so I'll try to follow the path of least resistance, which might mean merging this PR but removing the 12.10.0 test update that I had originally introduced.

[olaurino]

Note, if I merge this without adding a test for 12.10.0 and the CIAO builds work I will have proven this PR works with both <12.10.0 in standalone world and with 12.10.0 in CIAO world, which is more than promising anyway. At that point I'll hopefully just need to update the conda binary and get the new travis jobs to pass.