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

Add sensible error message for specutils loaders #1709

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

tbowers7
Copy link
Collaborator

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. That PR (astropy/specutils#1068), was
finally included in specutils version 1.12 (released 2023-10-17).

The specutils optional dependency has been updated to require this version.

tbowers7 added 5 commits July 31, 2023 10:02
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:   doc/releases/1.14.1dev.rst
	modified:   setup.cfg
@tbowers7 tbowers7 requested a review from kbwestfall October 20, 2023 18:37
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

Merging #1709 (795230d) into specutil_wave (4379d29) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@                Coverage Diff                @@
##           specutil_wave    #1709      +/-   ##
=================================================
- Coverage          41.07%   41.06%   -0.01%     
=================================================
  Files                190      190              
  Lines              43649    43652       +3     
=================================================
  Hits               17927    17927              
- Misses             25722    25725       +3     
Files Coverage Δ
pypeit/specutils/pypeit_loaders.py 0.00% <0.00%> (ø)

Copy link
Collaborator

@profxj profxj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

Just check that you have directed this PR where you intended.

@kbwestfall kbwestfall merged commit ac053fe into specutil_wave Oct 23, 2023
23 checks passed
@kbwestfall kbwestfall deleted the update_specutils_loaders branch October 23, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants