-
-
Notifications
You must be signed in to change notification settings - Fork 129
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
Convert an automatic registration into an option #1068
Conversation
Make registering a SpectrumList reader for any `data_loader` that reads Spectrum1D objects optional, per the TODO in `specutils/io/registers.py`.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1068 +/- ##
=======================================
Coverage 70.72% 70.72%
=======================================
Files 64 64
Lines 4485 4485
=======================================
Hits 3172 3172
Misses 1313 1313 ☔ View full report in Codecov by Sentry. |
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
This looks good to me, but I would call the option |
PR comment; change option to `autogenerate_spectrumlist`.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks. I just fixed the unrelated codestyle errors in another PR, they don't need to hold this up.
* Convert an automatic registration into an option Make registering a SpectrumList reader for any `data_loader` that reads Spectrum1D objects optional, per the TODO in `specutils/io/registers.py`. * Option name change PR comment; change option to `autogenerate_spectrumlist`.
Per the TODO suggestion in the code, this PR makes registering a SpectrumList reader for any
data_loader
that reads Spectrum1D objects optional (rather than automatic) via a boolean input flag. The default behavior is identical to the existing code, but the new flag allows users to opt out in situations where such registration causes errors.