Improve the XSPEC interface #2216
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2216 +/- ##
==========================================
+ Coverage 87.44% 87.48% +0.03%
==========================================
Files 84 84
Lines 29866 29924 +58
Branches 3834 3842 +8
==========================================
+ Hits 26116 26178 +62
+ Misses 3640 3635 -5
- Partials 110 111 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
DougBurke
force-pushed
the
xspec-use-12120-redux
branch
2 times, most recently
from
e8b3983 to
a044a56
Compare
Merged
DougBurke
force-pushed
the
xspec-use-12120-redux
branch
3 times, most recently
from
5bca9b4 to
660c554
Compare
DougBurke
force-pushed
the
xspec-use-12120-redux
branch
2 times, most recently
from
ad5aa48 to
511e0a0
Compare
Draft
Add a way to clear the XSET database - we already have one,
matching XSPEC, but it's not been advertised to the user, so
it makes sense to add a simple way to do it. The old way,
of calling `set_xsxset("INITIALIZE", ..)` has been marked as
deprecated.
Allow the full database to be returned, by calling get_xsxset
with no argument. This requires C++11 support.
We now raise an error if get_xsxset is called with an invalid
keyword, rather than returning an empty string, to better
match Python dictionaries.
The keywords should now be cleared if the "modelstrings"
entry is given (whether the empty dictionary or not), when
handling the xsstate method.
We can set the model path as well as the manager path, so do so. This introduces the ability to send in a path as an actual Path object, rather than just a string. There's a lot of discussion about "how much Path is too much", but here - these are not often used routines - the overhead of supporting Path as well as str is low There is the thought that we could return a Path rather than a str, but that would be a larger, breaking change. For get/set_xsstate, we could avoid tracking the paths settings, but then we can not identify the difference between the user explicitly setting the value to this is the default value. So we track the values in xsstate["paths"].
These values are included in the XSPEC state. User-level access is via get_xsversion and set_xsversion but the direct access is via individual routines.
We can now send an array of abnudances to the C++ layer rather than having to write the data to a file. This does not change the UI layer (at least, for users of the sherpa.astro.xspec module) but it simplifies some of the internals of that module. In particular, it makes the state handling a lot-more robust, as we can record the actual values set, and so re-create them if necessary. The get_xsabundances_path routine has been added: this is a simplification of the low-level API, which seperates the directory and file name, but for users of sherpa.astro.xspec it does not seem worth maintaining that separation. This routine is not automatically exported, as it is assumed to be rarely used. The naming scheme here is a bit confusing: does path mean a generic path or a Path object. We could also support changing the path or filename, but it's not obvious what the logic here is, in that changing the names does not cause the data to be re-loaded. There is currently no-way to access the cross-sections data as is done for the abundances.
It turns out if you try to set the abundance setting to "file" before reading in any data then you can crash XSPEC. So take advantage of FunctionUtility::abundChanged to track this (as this appears to be the reason for this method). An alternative would be to ban "file" from the set_xsabund call, but maybe people like switching between a file value and another table (although there are other ways to do this in Sherpa). I have also added some documentation to set_xsabund to explain why we do not just call readNewAbundances but essentially re-implement it (or, rather, now that we can use the C++ API we keep the original code, as the xsFORTRAN interface did not provide access to readNewAbundances). An alternative here would be to drop the file-reading option from the C++ API and make users read the data in Python and then use the set_xsabundances routine to load in the data. This would simplify the C++ code a bit *but* - it would break existing code - there is an advantage to having code that behaves as XSPEC does (in case there's subtle differences in reading in numbers and transferring them to the XSPEC internals)
The XSPEC API lets us ask for an abundance value from any of the
known tables, not just the selected one. So, how can we provide
this to the user? I have gone from having a very fine-grained
set of routines - e.g. providing access by name or by ion -
to a much simpler routine, where
- get_xsabundances can be used with or without a table name
- get_xsabund is stuck using the selected abundance table
I just don't think that users of Sherpa need that many ways to
get the data, and that
get_xsabundances(table)
should cover all their needs.
The code in sherpa/astro/xspec/src/_xspec.cc has been written
to make it easier to read - so we have the existing get_abund
routine and the new get_abund_from_table routine. The logic
could have been all placed in get_abund but
- it just complicates things that we don't need to complicate
- the sherpa.astro.xspec._xspec module is really an
internal-to-Sherpa module, so we can add/remove code here as
we want; it is the sherpa.astro.xspec interface we want to
try and keep backwards compatible
DougBurke
force-pushed
the
xspec-use-12120-redux
branch
from
511e0a0 to
f165e31
Compare
|
Rebased as #2215 has been merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Add and extend routines in sherpa.astro.xspec: get_xsabundances now takes an optional table name, get_xsxset can be called with no argument, set_xsversion has been added to allow the nei and atomdb versions to be changed, get_xsversion can return the nei and atomdb versions, and clear_xsxset will clear the XSET database.
Details
This PR builds on #2215
The replacement for #1615 (what's left of it). I will note that in working on this code I have simplified some of the choices, so the exact functionality doesn't match #1615, but I believe we have what we need and the extra options provided in the earlier PRs are not actually needed.
Some of these code changes require C++11. They could be rewritten to not use C++11 features, but it does not seem worth it as our expected compilers have long-supported these capabilities.
A number of routines have been added or changed in
sherpa.astro.xspec._xspecas this is treated as an "internal" module (i.e. if users are calling this directly then they have to expect changes). The user-level changes insherpa.astro.xspecare intended to be more backwards-compatible, as normally we have just added routines or added optional arguments. There is one breaking change however, noted below.Changes are:
clear_xsxsetas an "obvious" way to clear the XSXSET database (there was a hidden way to do this but you had to know XSPEC internals to do so, by setting the"INITIALIZE"key)get_xsxsetcan now be called with no name, in which case it returns a dict containing all the settingsget_xsxsetnow raises aKeyErrorif given an invalid name (to make the dict-like behaviour cleaner) rather than returning an empty stringset_xspath_managercan now be sent aPathor astrset_xspath_model(which can be sentPathorstr)get_xsversioncan now return theneiandatomdbversions instead of the library versionset_xsversionhas been added to allow theneiandatomdbversions to be changedset_xsabundancesnicer, but it's not visible to the user, but it does mean that if you have set abundances from a file (or adjusted the values) then they will be recorded in the "xspec state"get_xsabundances_pathas a way to get thePathto the abundances file used by XSPECset_xsabundC++ implementation has added comments to explain why we don't useFunctionUtility::readNewAbnudances()but still keep the code from when we only were able to use the xsFORTRAN interface (which does not have a "read abundances from a file" routine); basically the error handling and behaviour ofreadNewAbundancesmean it's not worth changing our existing codeget_xsabundances("lodd")etcThis leaves out three future changes that are being worked on (and were in #1615, apart from, the DEM change)