-
-
Notifications
You must be signed in to change notification settings - Fork 41
1D Wavelength Calibration #265
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #265 +/- ##
==========================================
+ Coverage 87.11% 89.52% +2.40%
==========================================
Files 15 17 +2
Lines 1281 1795 +514
==========================================
+ Hits 1116 1607 +491
- Misses 165 188 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ok, looks like all the builds are failing because of a matplotlib dependency that I added. I'll need to make matplotlib and the plotting functionality optional. |
|
Docs build works with Jupyter Notebook examples. Take a look at https://specreduce--265.org.readthedocs.build/en/265/wavelength_calibration/wavelength_calibration.html |
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.
I gave this a first pass and left a few comments. Notebook outputs need to be cleared, and i think it might make sense to have a data subdirectory rather than putting these files in docs
| "from matplotlib.pyplot import setp, subplots, close, rc\n", | ||
| "from specutils import Spectrum1D\n", | ||
| "\n", | ||
| "from specreduce.lswavecal1d import WavelengthSolution1D\n", |
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.
should be from specreduce import WavelengthCalibration1D if you want to import it in init like wavelengh_calibration, otherwise just a typo and should be specreduce.lswavecal1d import WavelengthSolution1D
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.
Thanks for catching this @cshanahan1. This notebook was outdated, and I shouldn't have included it in the PR in the first place.
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "flux = getdata('shane_kast_blue_600_4310_d55.fits', 1).astype('d')\n", |
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.
need to provide the path ../docs/wavelength_calibration/wavelength_calibration/, and also it looks like the file in that directory is different shane_kast_blue_600_4310_d55.fits so either change the filename to that one or add the correct file.
| ], | ||
| "source": [ | ||
| "%%time\n", | ||
| "ws = WavelengthSolution1D(arc_spectra=arc_spectrum, line_lists=[['CdI', 'HgI', 'HeI']], wlbounds=(3200, 5700))\n", |
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.
This class doesn't exist, if this should replaced with WavelengthCalibration1D and wlbounds needs to be line_list_bounds and ref_pixel should be supplied. Also remove %%time from the top of this cell. I got a traceback when I tried this replacement with 'ref_pix', so i'll wait to continue reviewing the rest of this notebook
i need to sit down and give this PR a proper review, but wanted to make a quick comment on this. i think GWCS should be the primary focus because it supports everything we will need to do while the FITS standard only really applies to the 1D case. the FITS standard for 2D spectra was never fully adopted or fleshed out and what exists is insufficient for our needs. as such, i think it's fine to only support GWCS for now. backwards compatibility for cases where it's possible can easily be added later. GWCS already provides a |
… a sum of nearest-line distances between a theoretical line list and a transformed observed line list.
…with respect to x.
Renamed `wavecal.py` to `lswavecal1d.py` and refactored the `WavelengthSolution1D` class for improved clarity and functionality. Added new methods like `refine_fit`, `pix_to_wav`, and `wav_to_pix`, and modified existing ones to enhance flexibility and maintainability.
Introduce `WavelengthSolution2D` class for 2D wavelength calibration, extending functionality from 1D calibration.
…el and spectral frames.
… type hints, documentation, and method structure. Added new methods for calculating inverse transformations and derivatives. Enhanced plotting functionality with additional parameters and refined docstrings for clarity.
…where `arc_spectra` is `None`.
… `plim` parameter for pixel range control in plotting.
…, and modularity, while resolving associated issues. Enhanced methods for handling pixel-to-wavelength mappings, line-matching processes, and plotting capabilities. Added new functions and properties for better control over data and fit refinement.
- Stopped catching warnings in `WavelengthCaliobration1D.find_lines`. - Revised the code style.
- Added `_line_match_distance` method - Renamed `fit_global` to `fit_dispersion` - Fixed bugs in setting the default higher order coefficient boundaries
…`_wcs` attribute in `wavecal1d.py`.
…e updates: - Introduced `degree` as an adjustable parameter in `fit_lines`, `refine_fit`, and `fit_dispersion`. - Removed fixed `degree` from the constructor, now initialized dynamically. - Updated `_init_model` to accept optional coefficients. - Improved handling of refined fits and degree validation via a `degree` property.
…s what comes to flux conservation and uncertainty handling. These should all be fixed now.
|
Thanks for your work, @kbwestfall. This was an excellent, thorough, and useful review! I've now (hopefully) addressed all your comments, so please feel free to take a look. |
…managing wavelength models and transformations.
- Introduced `WavelengthSolution1D` to manage pixel-to-wavelength transformations. - Updated references to `_solution` and `p2w` for clarity and consistency. - Added new tests for `WavelengthSolution1D`.
…operty type annotations.
…d related tests and docstrings. Expanded and clarified docstring for `WavelengthSolution1D` initialization.
- Replaced `_solution` with `solution` for clarity. - Updated return types of fit methods to return `WavelengthSolution1D`. - Reorganized parameter order and docstrings for consistency.
…lution1D` usage and examples: - Added `automodapi` entry for `specreduce.wavesol1d`. - Expanded explanations of 1D calibration workflow with `WavelengthSolution1D`. - Updated code examples to reflect new API.
…lines for wavelength calibration
This PR introduces a new 1D wavelength calibration class,
specreduce.wavecal1d.WavelengthCalibration1D, intended to fully replace the existingspecreduce.wavelength_calibration.WavelengthCalibration1Din version 2.0 (I'm keeping the previous class for backwards compatibility).The only (somewhat) significant feature currently missing is support for generating standard spectroscopic
-GRIand-GRAWCSs as described in Greisen et al. (2006). At the moment, the class supports only GWCS-based WCS. This is something we can address in a future update.