Add abstract base class for channels

[namurphy]

This PR will add a subpackage tentatively called abstractions which will include an abstract base class tentatively called AbstractChannel. This PR is so we can iterate upon the design initiated by @wtbarnes.

This PR comes after a discussion about how aiapy and xrtpy can have a common interface for calculating effective areas and temperature response functions. Having a common interface would be particularly useful for multi-instrument differential emission measures, and for cross-calibration between different instruments. This could potentially also be useful for other instrument packages like irispy.

This idea is a joint effort among @wtbarnes, @joyvelasquez, @nabobalis, @jslavin, and me.

@nabobalis & @wtbarnes: since you're package maintainers (I think), please feel free to edit this directly.

We can follow up on this with separate PRs for things like AbstractSpectralModel, or other things.

TODOs before merging:

• [x] Add a docstring for the class, with an example of how to use it.
• [x] Add docstrings for each of the abstract methods, once we decide on the interface
• [x] Decide if gain should be part of the interface
• [x] Add tentative parameters for each of the methods, with a type hint annotation for the appropriate astropy.time object? (I think we can do this, but that the parameters would essentially be suggestions and not requirements.) Should we include the spectral model as well?
  • [x] Double-check consistency with topic guide in #111
  • [x] Add API docs page for response
  • [x] Fix changelog
  • [ ] Add links to topic guide where appropriate in docstrings once #111 is merged.
  • [x] Test this with aiapy LM-SAL/aiapy#344
  • [ ] Test this with xrtpy

[namurphy]

Here's a tentative checklist for things to do:

• [x] Add a docstring for the class, with an example of how to use it.
• [x] Add docstrings for each of the abstract methods, once we decide on the interface
• [x] Decide if gain should be part of the interface
• [x] Add tentative parameters for each of the methods, with a type hint annotation for the appropriate astropy.time object? (I think we can do this, but that the parameters would essentially be suggestions and not requirements.) Should we include the spectral model as well?

[namurphy]

@nabobalis also brought up the idea of Python 3.8+ protocol classes, which would require making this package Python 3.8+ instead of 3.7+.

[dstansby]

@nabobalis also brought up the idea of Python 3.8+ protocol classes, which would require making this package Python 3.8+ instead of 3.7+.

It looks like this package is now 3.8+ if you want to take this approach!

[wtbarnes]

I've made a first pass at a full implementation of the AbstractChannel using the definitions of the terms laid out in #111. This still needs work, especially in terms of documenting each method.

Additionally, I've mixed abstract properties/methods with concrete implementations of several of the methods. I'm not sure if this is the right way to go about doing this? Should this go in a separate BaseChannel class that can then be subclassed?

I've also implemented a fairly simple SourceSpectra class. It uses xarray for the underlying data representation and essentially only exists to provide Quantity objects for the different axes. This should also probably get its own file at least.

Finally, I think this should not be merged until we've come to a consensus on #111.

[wtbarnes]

This should wait on #98 to be merged such that the appropriate links can be added to the docstrings. This abstract base class should also be tested out in several of the existing instrument packages like aiapy or xrtpy before being merged.

[wtbarnes]

TODOs before merging:

• [x] Double-check consistency with topic guide in #111
• [ ] Add API docs page for response
• [ ] Fix changelog
• [ ] Test this with aiapy and xrtpy

[wtbarnes]

I'm not the biggest fan of having an abstractions subpackage though I'm not sure what else to call it. It seems odd to have "abstractions" so high up in the namespace though since this is a feature that will be used by developers of other packages, not users.

Maybe we could have a response subpackage (where potentially some actual instrument responses could go) and then have abstractions.py under that?

[nabobalis]

Maybe we could have a response subpackage (where potentially some actual instrument responses could go) and then have abstractions.py under that?

I like this

[namurphy]

I'm not the biggest fan of having an abstractions subpackage though I'm not sure what else to call it. It seems odd to have "abstractions" so high up in the namespace though since this is a feature that will be used by developers of other packages, not users.

Now that you mention it, I agree!

Maybe we could have a response subpackage (where potentially some actual instrument responses could go) and then have abstractions.py under that?

Sounds great to me! Did you want to make that change?

[wtbarnes]

Sounds great to me! Did you want to make that change?

Yes, I'm happy to. I'm actively working on the branch at the moment anyway.

[wtbarnes]

The oldest deps build is failing because for some reason it is picking up xarray==0.7.0 which is from 8 years ago! https://github.com/pydata/xarray/releases/tag/v0.7.0

[wtbarnes]

Just a general comment - what about non-imaging instruments that could also use this type of implementation?

Yes, I think non-imaging instruments should be able to as well. This should presumably be applicable to any imaging instrument and in the context of just the wavelength-resolved effective area, may also be applicable to spectroscopically-resolved data as well. In the case of the temperature response, this should be applicable to any wavelength-integrated instrument primarily observing optically-thin emission.

Also it would be good to have an example added here to on how to use this

We could create a semi-contrived example in the docs. We could also point to how this is done in external packages like aiapy or xrtpy (once that actually happens). I see this as primarily a developer tool rather than a user tool so maybe just a silly contrived example is sufficient.

[wtbarnes]

We had some discussion about this on the community call today (9/25/24). There is a feeling that this should probably go into the core package (sunpy) at some point as the functionality is quite general and it would be more convenient for users to just depend on sunpy rather than having to depend on sunkit-instruments as well. However, there was some hesitancy about having xarray become a core dependency (even an optional one). Keeping this in sunkit-instruments for now also means we can iterate on it faster as this is still new functionality and the API may change as it gets used in the wild more frequently.

[Cadair]

+1 to getting this into core, I think we could have a discussion about the xarray thing when that happens if needed.