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

planned I/O changes for 4.17 #1939

Open
DougBurke opened this issue Dec 15, 2023 · 0 comments
Open

planned I/O changes for 4.17 #1939

DougBurke opened this issue Dec 15, 2023 · 0 comments

Comments

@DougBurke
Copy link
Contributor

DougBurke commented Dec 15, 2023

I have a number of planned changes to the I/O code and wanted to provide something to tie things together. We have

  • new code added in 4.16 (e.g. save_arf and save_rmf along with the experimental "write out an XSPEC table model" routines)
  • a lot of technical debt, since the I/O backends have not been updated to reflect changes in both crates and astropy
  • a number of annoying bugs where the two backends don't behave the same
    • note we can not guarantee bit-for-bit output from the two backends, in fact I claim that we can try to keep the data "semantically" the same but we are to always going to get things like the data formats exactly the same, but we want to better understand when they can be the same and when they can't be, and that's tricky to identify with the existing code
    • we occasionally hit the "backend a can read in this RMF file but backend b can not" and sometimes it's because of an issue with the backend, but not always
  • there has been work to define the I/O layer (i.e. that a backend would need to implement) but it is not as clear as it could be
    • we could follow the plotting approach and move to a class-based approach but I honestly don't see the benefit here compared to the existing module-based approach (since we are unlikely to want to subclass a particular backend)

I had been working on this for the 4.16 release, in particular working on supporting "multi matrix" RMF files, but decided that it wasn't going to be possible to get in on time. We now have time. There are two primary changes

  1. take advantage of the classes introduced in 4.16 to better represent FITS-like data to send data between the backend code and the generic IO code
  2. rewrite the code so that the conversion from FITS-like concepts to the Sherpa data objects (on input) and the reverse (on output) is done in the generic layer, with the backend code just translating between the on-disk format and the new FITS-like classes

In doing this we can pay down some of the technical debt since we are re-working the backend code. So the backend code is going to change (generally that the arguments for the existing calls will change) but the generic interface is not going to see any significant change (hopefully minimal changes), since this is the AI that users can expect to be using.

Code changes are, in order

  • Let users know they should use load_xstable_model not load_table_model #1926
    • this is a combination of "finally tell users that load_table_model should not be used with XSPEC models" (I'd like to just ban it now but people haven't got the message so I am not sure we want to do this) along with some minor code clean up and test coverage improvements based on the follow-up PRs
  • IO: separate pack and set commands #1929
    • I have never liked the "packup" argument for the I/O ocde and if you look at the implementation you can see it is like shoe-horning two different routines into one, so this breaks this (this one does actualy change the I/O API a bit, unlike my promise from earlier)
    • this PR "defines" an API for the dummy module (by making each routine have accept arguments more restrictive than "*args, **kwargs")
    • this has now been split up into
  • Clean up of the IO code #1921
    • this does the major work of moving the logic to
      1. use the new classes in the sherpa.astro.io.io_types module for sending data between the backend and generic layers
      2. move the conversion of FITS-like data to the Data classes into the generic layer, with the idea being that we should have a lot less "oh, but backend a can read this file when backend b can not" issues, and make it easier to see the output files are more-closely aligned for the two backends
  • Support multi matrix rmf #1869
    • this adds in support for multi-matrix RMF files
    • it still needs work, and I have not yet rebased this onto Clean up of the IO code #1921 - I may instead just create a new PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant