Boxcar extraction using Trace class

[ibusko]

This PR modifies the boxcar extraction code to directly interface with the Trace class in specreduce.

It also removes references to background extraction from the unit test module (test_extract.py), and adds a few unit tests to check corner cases involving fractional pixels.

Support for 2D "transposed" spectral images (where the dispersion axis is the first axis), and 3D spectral cubes, was partially added but not fully implemented and tested. Transposed spectral images are not yet supported by the Trace class, and 3D cubes were deemed low priority at this point.

Note that the tracing.py module currently does not provide any tool to actually find a trace in a spectral image (although it contains a function to build an image to be used in unit tests). In the original notebook the trace finding function was provided by the kosmos package.

A new notebook was added (notebook_sandbox/jwst_boxcar/boxcar_extraction.ipynb) to demonstrate the use of Trace and BoxcarExtract in practice.

Currently there is no support for a WCS, if present in the data.

[kecnry]

I'll plan to go through the notebook and some test-cases next week, but just wanted to add a few comments on the BoxcarExtract class itself.

[tepickering]

a few minor comments and requests. the most substantive one is the hard-coding of units in the output Spectrum1D. it will be encouraged to use CCDData as the input data format for specreduce which will allow for input data to contain flux units and uncertainty arrays. that information should be checked for and utilized if available.

[tepickering]

i am not sure why the devdeps test is failing. it doesn't look like it's related to anything we're doing. i will investigate further and apply hot fixes to main as needed.

[ojustino]

extract.py

1. I think we should either take each of image, trace_object, and width as arguments of BoxcarExtract's __call__()/__init__() or have them all be assigned as class attributes. Having some be set one way and some the other isn't intuitive to me.

I lean toward having them all as arguments of __call__() for increased flexibility with the BoxcarExtract object and since some other astropy utilities (e.g. modeling) also work this way. I'm also writing HorneExtract like this. Whichever route we go, consistency and intuitiveness should be our biggest concerns.

2. This is more germane to the upcoming tracing discussion (Spectral tracing discussion #83), but I think allowing for interchangeability of dispersion and cross-dispersion axes like we do here is better than assuming each is always on a specific axis. We don't have a mandate to write a full pipeline, so in my mind, that limits the number of assumptions we can make about the spectra we receive.

My ideal would be to store dispersion axis info in trace objects, which would make those arguments unneeded in BoxcarExtract. Barring that, I think the current approach here is OK.

3. What do we think of having _get_boxcar_weights() and _ap_weight_images() as class methods of BoxcarExtract instead of free-floating functions?

4. The previous iteration of BoxcarExtract had some plotting functionality. Does it make sense to include a rudimentary version of that here?

5. Some lines run long on characters and could be separated.

test_extract.py

Would it be useful to include real data as part of our tests? I can share several 2D VLT spectra alongside their pipeline's 1D extractions if so.

boxcar_extraction.ipynb

The notebook runs for me and I find the expository text at the beginning helpful.

1. I think it would be useful to see the pipeline 1D spectrum on the same plot as the extracted 1D spectrum at the end.
2. I suggest rearranging imports into a more standard order (froms, then regular imports, etc.)

[ibusko]

@ojustino thanks for the detailed review!

As for the simultaneous plot of extracted spectrum and pipeline result in the notebook: it doesn't work well because of the units in which each one is expressed. Until we add to the extraction code the ability to construct a wavelength scale (from WCS perhaps, or from user-supplied information of some sort), they are not directly comparable. I think the notebook can be reused when we implement that ability, and at that point one may create the plot as you suggest (and I agree with you that is what we really need, a simultaneous plot of both).

[tepickering]

looks like the devdeps failure resolved itself. it was affecting several other packages in the same way according to the convos i saw on the astropy slack...

[tepickering]

thanks for all this work, @ibusko, and for the detailed review, @ojustino! per @ojustino's points:

1. i completely agree here. those should be arguments to __call__() so that the same BoxcarExtract configuration can be reused for multiple images/traces/widths.
2. i'll follow up with my thoughts on this in Spectral tracing discussion #83.
3. _get_boxcar_weights() should definitely be a class method, i think, since its specifically related to how boxcar extraction is done. _ap_weight_image() looks more general purpose, though, and may be applicable to other extraction methods.
4. check plots are nice to have. on the other hand, a Spectrum1D object is being returned which has its own built-in plot() method. that covers the use case of viewing extracted flux vs pixel position along the trace.
5. the codestyle test that CI runs is configured to use a max line length of 100. i'm open to changing that, but setting it too low can lead to other readability issues, in my opinion...

[kecnry]

_get_boxcar_weights() should definitely be a class method, i think, since its specifically related to how boxcar extraction is done. _ap_weight_image() looks more general purpose, though, and may be applicable to other extraction methods.

_ap_weight_image calls _get_boxcar_weights so for now I moved them nested into the __call__. If we need them elsewhere down the road, we can move them out or to class methods.

I think this is now ready for re-(final?)-review (assuming CI passes)

[PatrickOgle]

Tried out boxcar_extraction.ipynb:

1. Basic functionality is there-input image, trace, width and get out spectrum
2. Output flux is a factor of exactly 10^6 smaller than the comparison x1d product. Not sure why...maybe a function of post-extraction flux calibration applied to the x1d product.
3. Basic extractions should be unitless, anyway--counts vs. pixel. Wavelength and flux calibration get applied later.

[kecnry]

Output flux is a factor of exactly 10^6 smaller than the comparison x1d product. Not sure why...maybe a function of post-extraction flux calibration applied to the x1d product.

That's a good point - we did pull the following out from the code when migrating from the notebook. I'm not sure where this factor best belongs (somewhere in the code without the factor being hardcoded for JWST or maybe in the boxcar_extraction.ipynb notebook)?

# convert from MJy/sr to Jy
ext1d *= 1e6 * jdatamodel.meta.photometry.pixelarea_steradians

[tepickering]

CI has passed and i think we're in agreement on the changes that have been made. as far as i'm concerned, this is good to merge. there's more work to do, but it depends on tracing functionality being fleshed out further which is work-in-progress.

[ojustino]

I agree and also approve -- looks like we've addressed everything that was immediately crucial.

[kecnry]

Also happy to merge, thanks everyone!