Add a function to measure cross-dispersion profile #214
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
+ Coverage 82.16% 83.14% +0.98%
==========================================
Files 10 12 +2
Lines 1015 1086 +71
==========================================
+ Hits 834 903 +69
- Misses 181 183 +2 ☔ View full report in Codecov by Sentry. |
| along the dispersion axis. | ||
|
|
||
| If a single number is specified for `pixel`, then the profile at that pixel | ||
| (i.e wavelength) will be returned. If several pixels are specified in a list or |
There was a problem hiding this comment.
If pixels are mapped to wavelength, can user input the wavelength directly? Does it need units?
There was a problem hiding this comment.
my take is that the core routine for this should work in pixel space. if we want to work in wavelength space, then we can wrap or decorate this to accept a WCS and input wavelengths to determine pixel and pixel_range.
There was a problem hiding this comment.
are you ok with that being a follow up effort?
There was a problem hiding this comment.
Do we want this as a method on any of the extraction steps, perhaps on trace itself? If there are other uses, it makes sense to have the general capability as a standalone function, but it might be convenient to be able to call trace.cross_dispersion_profile and/or trace.plot_cross_dispersion_profile(), etc.
|
@camipacifici @kecnry this is ready for a final review - i added some of the tests for a non-flat trace with and without aligning the image along the trace. the only outstanding item is the pixels vs wavelengths thing. right now, since its clear that the input is pixels, i personally think its OK to add wavelength as another parameter (that can't be set simultaneously with pixels) later on. what do you think? |
|
test data with a non-flat trace: the returned profile with align_along_trace = False, at different pixels. the center moves as expected: And with align_along_trace = True - since the trace is rolled to the nearest integer value, the center for the profile is the same + or - one pixel: and when all pixels are used to measure the average profile for the whole image: |
cshanahan1
force-pushed
the
crossdisp_profile
branch
from
fb05298
to
f9176b4
Compare
|
Looks great, thank you! |
| @@ -1,3 +1,5 @@ | |||
| """ | |||
| General purpose utilities for specreduce | |||
| """ | |||
|
|
|||
| from .utils import * # noqa | |||
There was a problem hiding this comment.
do we want relative imports here? i don't have strong feelings personally, but i've been chided in the past for using them...
There was a problem hiding this comment.
I don't have an opinion either... we can always change this later if we come up with a policy/standard we want to follow, right?
There was a problem hiding this comment.
well, the PEP8 guide recommends absolute imports, fwiw: https://peps.python.org/pep-0008/#imports.
|
merging this - wavelength input can be addressed in a follow up |
This PR adds a function to measure the cross-dispersion profile at a single wavelength, or the average profile in a set/range of wavelengths. It takes in a 'width' for the window in the cross dispersion axis, which moves around the trace. A 1D array is returned with either the profile at that single wavelength/pixel or the mean or median profile across several wavelength/pixels.