Allow a way to circumvent monotonic wavelength requirement in Spectrum1D #1708
Conversation
When users try to ingest a `spec1d` file into a Spectrum1D object, the current scheme ends badly with an obscure error message like: ``` OSError: Could not identify column containing the wavelength, frequency or energy ``` Because PypeIt `spec1d` files may have multiple extensions, by convention we load these into SpectrumList objects in specutils, even if there is only a single spectrum. This commit catches the case when a user tries to load a `spec1d` file into a Spectrum1D object. Previously, such an attempt ended up with specutils loading in the `spec1d` file as a tabular FITS format, then didn't know what to do about the spectral axis (leading to the above obscure error). This commit specifically includes a loader for `spec1d` files into Spectrum1D objects, but emits a sensible error message in this case rather than trying to actually load the file. As it happens, in order to properly do this, I needed to make an adjustment in specutils itself. A PR has been submitted (astropy/specutils#1068), but this commit should not be considered as a PypeIt PR until the specutils change has incorporated into an official release. At that time, the specutils optional dependency will need to have a minimum version specified. So... this commit will quietly sit in a branch until specutils makes its move. modified: pypeit/specutils/pypeit_loaders.py
modified: setup.cfg
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## develop #1708 +/- ##
===========================================
- Coverage 41.08% 41.07% -0.02%
===========================================
Files 190 190
Lines 43631 43649 +18
===========================================
Hits 17927 17927
- Misses 25704 25722 +18
|
modified: doc/releases/1.14.1dev.rst modified: setup.cfg
| Spectrum wavelengths | ||
| flux : `numpy.ndarray`_ | ||
| Spectrum flux | ||
| ivar : `numpy.ndarray`_ |
There was a problem hiding this comment.
should we make it an optional argument, then? ivar=None
There was a problem hiding this comment.
I think if this function were moved to somewhere in core to be used outside of this loader, then making this argument optional is fine. Otherwise, this is just a helper function for use within this module and it doesn't matter since ivar in some form will be passed in all the time.
There was a problem hiding this comment.
Although @rcooke-ast 's suggestion is perfectly reasonable, I agree with @tbowers7 : this is really just a helper function that isn't intended to be used outside this module. To signal this is a "private" function, I added a leading underscore to the function name.
pypeit/specutils/pypeit_loaders.py
Outdated
| # want to be more acceptable, we should consider instead fitting a low-order | ||
| # polynomial to the pixel vs. wavelength function and rejecting strong | ||
| # outliers. | ||
| _wave = wave.copy() |
There was a problem hiding this comment.
I think all of the wave below this line should be changed to _wave
EDIT: Actually it is probably fine, since you pass in and set wave in pypeit_onespec_loader()... but I still flag this just in case
There was a problem hiding this comment.
I think I agree with @rcooke-ast 's first comment, else _wave doesn't do anything here.
There was a problem hiding this comment.
Good catch, and apologies. This was a leftover line I used during testing. Removing the line fixes the issue.
| @@ -134,3 +136,32 @@ def test_spec1d_io(): | |||
|
|
|||
| ofile.unlink() | |||
|
|
|||
|
|
|||
| @specutils_required | |||
| Spectrum wavelengths | ||
| flux : `numpy.ndarray`_ | ||
| Spectrum flux | ||
| ivar : `numpy.ndarray`_ |
There was a problem hiding this comment.
I think if this function were moved to somewhere in core to be used outside of this loader, then making this argument optional is fine. Otherwise, this is just a helper function for use within this module and it doesn't matter since ivar in some form will be passed in all the time.
pypeit/specutils/pypeit_loaders.py
Outdated
| # want to be more acceptable, we should consider instead fitting a low-order | ||
| # polynomial to the pixel vs. wavelength function and rejecting strong | ||
| # outliers. | ||
| _wave = wave.copy() |
There was a problem hiding this comment.
I think I agree with @rcooke-ast 's first comment, else _wave doesn't do anything here.
Add sensible error message for `specutils` loaders
As titled.
This only affects the specutils interface. Tests pass. No need for a dev-suite run.