-
Notifications
You must be signed in to change notification settings - Fork 7
Check_spectrum_plottable fix for JWST data in Jy #151
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
Conversation
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.
Pull Request Overview
This PR fixes the issue with check_spectrum_plottable for JWST data in Jy by updating the flux unit check and improving the handling of specutils Spectrum objects. The changes include replacing Spectrum1D with Spectrum along with adding underscored helper functions, parameterizing tests for multiple spectrum files, and updating logging messages.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/test_spectra.py | Updated tests to use Spectrum instead of Spectrum1D and parameterized test cases for JWST data. |
tests/conftest.py | Added the ignore_ads parameter to the ingest_publication call. |
astrodb_utils/utils.py | Changed log level for search messages and refined logging of instrument ingestion. |
astrodb_utils/spectra.py | Refactored spectrum checking functions to work with Spectrum; improved flux unit handling. |
astrodb_utils/init.py | Updated logger instantiation and initialization messages. |
Comments suppressed due to low confidence (1)
astrodb_utils/spectra.py:168
- The xlabel string will not interpolate the variable 'spectrum.spectral_axis.unit' because it lacks an f-string prefix. Consider changing it to an f-string (e.g., f"Dispersion ({spectrum.spectral_axis.unit})") for proper variable substitution.
plt.xlabel("Dispersion ({spectrum.spectral_axis.unit})")
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.
Pull Request Overview
This PR fixes issues with check_spectrum_plottable for JWST data in Jy by updating flux unit conversions and handling of tabular-fits files. Key changes include:
- Replacing Spectrum1D with Spectrum in tests and core functions.
- Adding parameterized tests to include JWST and WISE data.
- Updating logging messages and refining unit checks.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_spectra.py | Updated tests to use Spectrum, added parameterization for multiple files |
tests/conftest.py | Modified call to ingest_publication with an extra ignore_ads argument |
pyproject.toml | Updated the specutils dependency version for compatibility |
astrodb_utils/utils.py | Adjusted logging level and messaging for instrument ingestion |
astrodb_utils/spectra.py | Refactored spectrum check functions and improved docstrings |
astrodb_utils/init.py | Revised logger initialization message for clarity |
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! I see that you combined check spectrum plottable and check spectrum class and I agree with the reorganization.
Check_spectrum_plottable is currently not working for JWST data in Jy for two reasons:
This PR should fix the flux unit check and provide a work around for the tabular-fits auto-identify.
(Also working with
specutils
team to address the tabular-fits identify problem. astropy/specutils#1234)