Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor xspec additions #2215

Merged
merged 7 commits into from
Feb 13, 2025
Merged

Minor xspec additions #2215

merged 7 commits into from
Feb 13, 2025

Conversation

DougBurke
Copy link
Contributor

@DougBurke DougBurke commented Jan 24, 2025

Summary

Minor changes to the XSPEC module, including added documentation to the set_xsabund routine to note the availability of the "felc" table, and set_xsstate no-longer requires all fields to be present.

Details

This is a small subset of the changes from #1615 that do not require changing the C++ interface. The changes are (not in order, and not per commit):

  • an internal change to the tests so that mypy is less picky
  • the read_xstable_model routine can now be sent a filename as a Path object as well as a string; this is an "internal" routine so it is not going to change most people's lives
  • set_xsabund documentation is updated to mention the "felc" table
  • get_xsabund has an extra example in its docstring
  • updated the typing in sherpa/astro/xspec/__init.py to use Python 10 syntax (can avoid Union and Optional)
  • set_xsstate used to require all settings are present before it would run, which can be useful, but it also means that if we add a new field (as I plan to) , or delete an old field, then old states can not be restored because it wouldn't know about the new settings. So we now allow each setting to be optional.

@DougBurke DougBurke marked this pull request as draft January 24, 2025 21:30
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 85.10638% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.47%. Comparing base (c00eeff) to head (f002119).
Report is 18 commits behind head on main.

Files with missing lines Patch % Lines
sherpa/astro/xspec/__init__.py 85.10% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2215      +/-   ##
==========================================
+ Coverage   87.44%   87.47%   +0.02%     
==========================================
  Files          84       84              
  Lines       29848    29860      +12     
  Branches     3833     3831       -2     
==========================================
+ Hits        26101    26119      +18     
+ Misses       3637     3631       -6     
  Partials      110      110              
Files with missing lines Coverage Δ
sherpa/astro/xspec/__init__.py 98.25% <85.10%> (-0.07%) ⬇️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c00eeff...f002119. Read the comment docs.

There is no functional change to the tests, it is just the way
the tests are written (the issue is whether the ui symbol is
known to have the load_[xs]table_model routines).
DougBurke and others added 3 commits January 31, 2025 10:10
Change the test_set_xsstate_missing_key so that it called once
per key to check, using a parametrized argument, rather than
do them all at once. This makes the code slightly cleaner (no need
to copy the test dict) and allows testing all keys even when one
fails, at the expense of probably being slightly slower.
Since the return type depends on the arguments sent to the routine,
we can provide more-precise typing. Update to use Python 3.10
syntax (e.g. no need for Optional and Union).

It's not 100% clear from the typing documentation whether you should,
or need to, type the non-overloaded definition (I am reading proposed
changes to the documentation that doesn't make it clear); however, if
you don't set them then the annotations do not get set and so they
don't appear in help output. It also avoids an error reported by
pyright, so it seems worth doing.

Also add an example to the documentation for get_xsabund.

In looking at the XSPEC abundances, I noted that we did not document
the "felc" table, so do so for set_xsabund. In doing this I
re-arranged the list in the documentation so it's in alphabetical
order (ascending).
Allow the filename argument to read_xstable_model to be a Path
object as well as a str. To support typing this code I changed
the creation of the XSTableModel so that the call is explicit
(in matching the data that has just been read in to the
constructor call) rather than just using *cols.

There should be no functional change in behavior here.
@DougBurke DougBurke force-pushed the minor-xspec-additions branch from eeaf7a5 to 40a509a Compare January 31, 2025 15:35
@DougBurke DougBurke requested a review from hamogu January 31, 2025 16:21
Previously set_xsstate required all the settings to be present before
it could work. This has its advantages, but does limit things as you
can't restore the state if Sherpa has added a new field. So, we now
just process those fields that are available and skip those that are
not.

As part of this work the internals to the XSPEC module have changed
to use a single dictionary for storing the data we need to record the
XSPEC state, rather than having multiple (two) variables for this.
This is in part because the current XSPEC interface that Sherpa uses
does not allow Sherpa to ask XSPEC what its state is, so the code has
to try and record these changes.
@DougBurke DougBurke force-pushed the minor-xspec-additions branch from 40a509a to 8fb00fe Compare January 31, 2025 19:04
sherpa/astro/xspec/tests/test_xspec_unit.py Outdated Show resolved Hide resolved
Change the name from 'key' to 'miss_key', to make sure it is
distinct from 'key', which gets its value changed within the
routine.
@DougBurke
Copy link
Contributor Author

@hamogu - I made a small change to a test since you accepted it (found when working on #2216 but worth cleaning up here). Can you just give the last commit a quick check? Ta

@DougBurke DougBurke mentioned this pull request Feb 10, 2025
@hamogu
Copy link
Contributor

hamogu commented Feb 10, 2025

I don't see any functional change, only a renaming from key to miss_key. I mean, who not? But what's the intention?

@DougBurke
Copy link
Contributor Author

Because it separates the input argument (that is parametrized, aka miss_key) from the generic variable name key. So I can use key in the rest of the loop and not get strange behaviour (as ended up happening in later work where the code would fail because I'd not separated the two concepts).

@hamogu
Copy link
Contributor

hamogu commented Feb 10, 2025

Makes sense - and that subtlety also explains why I didn't notice the change on review, when Gh highlights just the changes lines and not the others where the word "key" also appears.

@wmclaugh wmclaugh merged commit 153afca into sherpa:main Feb 13, 2025
18 checks passed
@DougBurke DougBurke deleted the minor-xspec-additions branch February 13, 2025 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants