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

Implement 1D wavelength calibration classes #162

Merged
merged 31 commits into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d58f652
Working on line pixel refinement
rosteen Jan 18, 2023
1f3483a
Testing/debugging CalibrationLine, starting on WavelengthCalibration
rosteen Jan 19, 2023
c54aed7
Working on model fitting and construction of resulting GWCS
rosteen Jan 26, 2023
5eb4aaa
Working through bugs/testing
rosteen Jan 26, 2023
baa706c
Working initial implementation
rosteen Jan 26, 2023
02ed67e
Codestyle
rosteen Jan 26, 2023
1df8621
Match order of pixel/wavelength between CalibrationLine and Wavelengt…
rosteen Jan 27, 2023
eb5feea
Fix codestyle in test
rosteen Jan 27, 2023
6ea98e4
Adding tests for CalibrationLine
rosteen Jan 31, 2023
af4a523
Fix codestlye
rosteen Jan 31, 2023
568fd09
Increase atol for gaussian center
rosteen Jan 31, 2023
be35c09
Remove autoidentify for now, add Polynomial1D test
rosteen Feb 1, 2023
f3b1b9f
Add cache clearing, avoid AstropyUserWarning
rosteen Feb 1, 2023
8117864
Add input_spectrum property and clear cache on setting it
rosteen Feb 1, 2023
8f1535b
Add default range if method is set but not range, remove check for un…
rosteen Feb 2, 2023
22e2d68
Also clear cache when updating lines or model
rosteen Feb 3, 2023
4a508e3
Move fitter default to wcs call
rosteen Feb 3, 2023
a8ca23f
Keep storing fitter as None
rosteen Feb 3, 2023
1941a26
Add docs, improve importing of new classes
rosteen Feb 7, 2023
829dc5c
Remove CalibrationLine, move to astropy tables internallyt
rosteen Apr 6, 2023
c3ba30d
Codestyle
rosteen Apr 6, 2023
62c8025
Apply suggestions from code review
rosteen Apr 7, 2023
12a8661
Remove speactral_units arg, add docstring for fitter arg
rosteen Apr 7, 2023
5063b5e
Update documentation, add NotImplementedError for catalog input
rosteen Apr 7, 2023
b6a92f8
Reverse sort frequencies
rosteen Apr 7, 2023
1aa62c6
Codestyle
rosteen Apr 7, 2023
7f31b61
Increase test coverage
rosteen Apr 10, 2023
45939a9
Fix tests and bugs found by tests
rosteen Apr 10, 2023
f92bac5
Codestyle
rosteen Apr 10, 2023
d573219
Add QTable to catalog options
rosteen Apr 10, 2023
82170a5
Finish addressing review comments
rosteen Apr 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@
{
'astropy': ('https://docs.astropy.org/en/stable/', None),
'ccdproc': ('https://ccdproc.readthedocs.io/en/stable/', None),
'specutils': ('https://specutils.readthedocs.io/en/stable/', None)
'specutils': ('https://specutils.readthedocs.io/en/stable/', None),
'gwcs': ('https://gwcs.readthedocs.io/en/stable/', None)
}
)
#
Expand Down
1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ Calibration
.. toctree::
:maxdepth: 1

wavelength_calibration.rst
extinction.rst
specphot_standards.rst

Expand Down
52 changes: 52 additions & 0 deletions docs/wavelength_calibration.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
.. _wavelength_calibration:

Wavelength Calibration
======================

Wavelength calibration is currently supported for 1D spectra. Given a list of spectral
lines with known wavelengths and estimated pixel positions on an input calibration
spectrum, you can currently use ``specreduce`` to:

#. Fit an ``astropy`` model to the wavelength/pixel pairs to generate a spectral WCS
solution for the dispersion.
#. Apply the generated spectral WCS to other `~specutils.Spectrum1D` objects.

1D Wavelength Calibration
-------------------------

The `~specreduce.wavelength_calibration.WavelengthCalibration1D` class can be used
to fit a dispersion model to a list of line positions and wavelengths. Future development
will implement catalogs of known lamp spectra for use in matching observed lines. In the
example below, the line positions (``line_list``) have already been extracted from
``lamp_spectrum``::

import astropy.units as u
from specreduce import WavelengthCalibration1D
line_list = [10, 22, 31, 43]
wavelengths = [5340, 5410, 5476, 5543]*u.AA
test_cal = WavelengthCalibration1D(lamp_spectrum, line_list,
line_wavelengths=wavelengths)
calibrated_spectrum = test_cal.apply_to_spectrum(science_spectrum)

The example above uses the default model (`~astropy.modeling.functional_models.Linear1D`)
to fit the input spectral lines, and then applies the calculated WCS solution to a second
spectrum (``science_spectrum``). Any other ``astropy`` model can be provided as the
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe specify "any 1D model"

input ``model`` parameter to the `~specreduce.wavelength_calibration.WavelengthCalibration1D`.
In the above example, the model fit and WCS construction is all done as part of the
``apply_to_spectrum()`` call, but you could also access the `~gwcs.wcs.WCS` object itself
by calling::

test_cal.wcs

The calculated WCS is a cached property that will be cleared if the ``line_list``, ``model``,
or ``input_spectrum`` properties are updated, since these will alter the calculated dispersion
fit.

You can also provide the input pixel locations and wavelengths of the lines as an
`~astropy.table.QTable`, with columns ``pixel_center`` and ``wavelength``::

from astropy.table import QTable
pixels = [10, 20, 30, 40]*u.pix
wavelength = [5340, 5410, 5476, 5543]*u.AA
line_list = QTable([pixels, wavelength], names=["pixel_center", "wavelength"])
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: i'd call this matched_line_list so as not to be confused with linelist or line_list which is usually understood to mean something else (i.e. list of wavelengths of lines due to specific ion/molecule or mixture of of them).

test_cal = WavelengthCalibration1D(lamp_spectrum, line_list)
1 change: 1 addition & 0 deletions specreduce/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@
# ----------------------------------------------------------------------------

from specreduce.core import * # noqa
from specreduce.wavelength_calibration import * # noqa
35 changes: 35 additions & 0 deletions specreduce/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
import os

from astropy.version import version as astropy_version
import astropy.units as u
import numpy as np
import pytest
from specutils import Spectrum1D

# For Astropy 3.0 and later, we can use the standalone pytest plugin
if astropy_version < '3.0':
Expand All @@ -20,6 +24,37 @@
ASTROPY_HEADER = False


@pytest.fixture
def spec1d():
np.random.seed(7)
flux = np.random.random(50)*u.Jy
sa = np.arange(0, 50)*u.pix
spec = Spectrum1D(flux, spectral_axis=sa)
return spec


@pytest.fixture
def spec1d_with_emission_line():
np.random.seed(7)
sa = np.arange(0, 200)*u.pix
flux = (np.random.randn(200) +
10*np.exp(-0.01*((sa.value-130)**2)) +
sa.value/100) * u.Jy
spec = Spectrum1D(flux, spectral_axis=sa)
return spec


@pytest.fixture
def spec1d_with_absorption_line():
np.random.seed(7)
sa = np.arange(0, 200)*u.pix
flux = (np.random.randn(200) -
10*np.exp(-0.01*((sa.value-130)**2)) +
sa.value/100) * u.Jy
spec = Spectrum1D(flux, spectral_axis=sa)
return spec


Comment on lines +27 to +57
Copy link
Member

Choose a reason for hiding this comment

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

would this benefit from using the synthetic spectra from #165?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I think using that to expand/improve the tests can be another PR once both of these are merged.

def pytest_configure(config):

if ASTROPY_HEADER:
Expand Down
54 changes: 54 additions & 0 deletions specreduce/tests/test_wavelength_calibration.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
from numpy.testing import assert_allclose

from astropy.table import QTable
import astropy.units as u
from astropy.modeling.models import Polynomial1D
from astropy.tests.helper import assert_quantity_allclose

from specreduce import WavelengthCalibration1D


def test_linear_from_list(spec1d):
centers = [0, 10, 20, 30]
w = [5000, 5100, 5198, 5305]*u.AA
test = WavelengthCalibration1D(spec1d, centers, line_wavelengths=w)
spec2 = test.apply_to_spectrum(spec1d)

assert_quantity_allclose(spec2.spectral_axis[0], 4998.8*u.AA)
assert_quantity_allclose(spec2.spectral_axis[-1], 5495.169999*u.AA)


def test_linear_from_table(spec1d):
centers = [0, 10, 20, 30]
w = [5000, 5100, 5198, 5305]*u.AA
table = QTable([centers, w], names=["pixel_center", "wavelength"])
test = WavelengthCalibration1D(spec1d, table)
spec2 = test.apply_to_spectrum(spec1d)

assert_quantity_allclose(spec2.spectral_axis[0], 4998.8*u.AA)
assert_quantity_allclose(spec2.spectral_axis[-1], 5495.169999*u.AA)


def test_poly_from_table(spec1d):
# This test is mostly to prove that you can use other models
centers = [0, 10, 20, 30, 40]
w = [5005, 5110, 5214, 5330, 5438]*u.AA
table = QTable([centers, w], names=["pixel_center", "wavelength"])

test = WavelengthCalibration1D(spec1d, table, model=Polynomial1D(2))
test.apply_to_spectrum(spec1d)

assert_allclose(test.model.parameters, [5.00477143e+03, 1.03457143e+01, 1.28571429e-02])


def test_replace_spectrum(spec1d, spec1d_with_emission_line):
centers = [0, 10, 20, 30]*u.pix
w = [5000, 5100, 5198, 5305]*u.AA
test = WavelengthCalibration1D(spec1d, centers, line_wavelengths=w)
# Accessing this property causes fits the model and caches the resulting WCS
test.wcs
assert "wcs" in test.__dict__

# Replace the input spectrum, which should clear the cached properties
test.input_spectrum = spec1d_with_emission_line
assert "wcs" not in test.__dict__
200 changes: 200 additions & 0 deletions specreduce/wavelength_calibration.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
from astropy.modeling.models import Linear1D
from astropy.modeling.fitting import LMLSQFitter, LinearLSQFitter
from astropy.table import QTable, hstack
import astropy.units as u
from functools import cached_property
from gwcs import wcs
from gwcs import coordinate_frames as cf
import numpy as np
from specutils import Spectrum1D


__all__ = ['WavelengthCalibration1D']


def get_available_catalogs():
Copy link
Contributor

Choose a reason for hiding this comment

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

thus far there is built-in access to some legacy line lists/line catalogs in the specreduce-data repository and more well-curated ones in pypeit. since either way would require spidering through remote directories to find what's there, i think this is probably better left to the documentation rather than a function like this. you need to know some details about which one to use because they can be very instrument-dependent.

actually, i forgot that i did make a global variable, PYPEIT_CALIBRATION_LINELISTS, that does list which ones are available that we'd want to use. so after #165 is merged, this could just return that.

"""
ToDo: Decide in what format to store calibration line catalogs (e.g., for lamps)
and write this function to determine the list of available catalog names.
"""
return []


def concatenate_catalogs():
"""
ToDo: Code logic to combine the lines from multiple catalogs if needed
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

i already implemented this in #165 for pypeit line lists. if you give a list of supported ions/molecules, it generates a combined line list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, we'll definitely need a PR after these are both merged to combine their powers 😃



class WavelengthCalibration1D():

def __init__(self, input_spectrum, line_list, line_wavelengths=None, catalog=None,
model=Linear1D(), fitter=None):
"""
input_spectrum: `~specutils.Spectrum1D`
A one-dimensional Spectrum1D calibration spectrum from an arc lamp or similar.
line_list: list, array, `~astropy.table.QTable`
Copy link
Contributor

Choose a reason for hiding this comment

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

i would call this line_pixels to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought here was that the user can provide the whole table, including both pixels and wavelengths, in which case it doesn't make sense to call it 'line_pixels. Maybe I'll change this slightly to have separate line_list and line_pixels arguments, where the former can be the whole table, and the latter is intended for just pixel values to then put internally into the table.

List or array of line pixel locations to anchor the wavelength solution fit.
Will be converted to an astropy table internally if a list or array was input.
Can also be input as an `~astropy.table.QTable` table with (minimally) a column
named "pixel_center" and optionally a "wavelength" column with known line
wavelengths populated.
line_wavelengths: `~astropy.units.Quantity`, `~astropy.table.QTable`, optional
`astropy.units.Quantity` array of line wavelength values corresponding to the
line pixels defined in ``line_list``. Does not have to be in the same order]
(the lists will be sorted) but does currently need to be the same length as
line_list. Can also be input as an `~astropy.table.QTable` with (minimally)
a "wavelength" column.
catalog: list, str, optional
The name of a catalog of line wavelengths to load and use in automated and
template-matching line matching.
Copy link
Member

Choose a reason for hiding this comment

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

could this be moved to a class method? So WavelengthCalibration1D.from_line_list(line_list) calls WavelengthCalibration1D(...)?

Copy link
Contributor

Choose a reason for hiding this comment

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

what i implemented in #165 returns a line catalog as a QTable. so this should be able to accept that and make sure there's a wave or wavelength column.

model: `~astropy.modeling.Model`
The model to fit for the wavelength solution. Defaults to a linear model.
fitter: `~astropy.modeling.fitting.Fitter`, optional
The fitter to use in optimizing the model fit. Defaults to
`~astropy.modeling.fitting.LinearLSQFitter` if the model to fit is linear
or `~astropy.modeling.fitting.LMLSQFitter` if the model to fit is non-linear.
"""
rosteen marked this conversation as resolved.
Show resolved Hide resolved
self._input_spectrum = input_spectrum
self._model = model
self._line_list = line_list
self._cached_properties = ['wcs',]
self.fitter = fitter
self._potential_wavelengths = None
self._catalog = catalog

# ToDo: Implement having line catalogs
self._available_catalogs = get_available_catalogs()

if isinstance(line_list, (list, np.ndarray)):
self._line_list = QTable([line_list], names=["pixel_center"])

if self._line_list["pixel_center"].unit is None:
self._line_list["pixel_center"].unit = u.pix

# Make sure our pixel locations are sorted
self._line_list.sort("pixel_center")

if (line_wavelengths is None and catalog is None
and "wavelength" not in self._line_list.columns):
raise ValueError("You must specify at least one of line_wavelengths, "
"catalog, or 'wavelength' column in line_list.")

# Sanity checks on line_wavelengths value
if line_wavelengths is not None:
if "wavelength" in line_list:
raise ValueError("Cannot specify line_wavelengths separately if there is"
"a 'wavelength' column in line_list.")
if len(line_wavelengths) != len(line_list):
raise ValueError("If line_wavelengths is specified, it must have the same "
"length as line_pixels")
if not isinstance(line_wavelengths, (u.Quantity, QTable)):
raise ValueError("line_wavelengths must be specified as an astropy.units.Quantity"
"array or as an astropy.table.QTable")
if isinstance(line_wavelengths, u.Quantity):
# Ensure frequency is descending or wavelength is ascending
if str(line_wavelengths.unit.physical_type) == "frequency":
line_wavelengths[::-1].sort()
else:
line_wavelengths.sort()
self._line_list["wavelength"] = line_wavelengths
elif isinstance(line_wavelengths, QTable):
line_wavelengths.sort("wavelength")
self._line_list = hstack(self._line_list, line_wavelengths)

# Parse desired catalogs of lines for matching.
if catalog is not None:
# For now we avoid going into the later logic and just throw an error
raise NotImplementedError("No catalogs are available yet, please input "
"wavelengths with line_wavelengths or as a "
"column in line_list")
if isinstance(catalog, list):
self._catalog = catalog
else:
self._catalog = [catalog]
for cat in self._catalog:
if isinstance(cat, str):
if cat not in self._available_catalogs:
raise ValueError(f"Line list '{cat}' is not an available catalog.")

# Get the potential lines from any specified catalogs to use in matching
self._potential_wavelengths = concatenate_catalogs(self._catalog)
rosteen marked this conversation as resolved.
Show resolved Hide resolved

def identify_lines(self):
"""
ToDo: Code matching algorithm between line pixel locations and potential line
wavelengths from catalogs.
"""
pass

def _clear_cache(self, *attrs):
"""
provide convenience function to clearing the cache for cached_properties
"""
if not len(attrs):
attrs = self._cached_properties
for attr in attrs:
if attr in self.__dict__:
del self.__dict__[attr]

@property
def available_catalogs(self):
return self._available_catalogs

@property
def input_spectrum(self):
return self._input_spectrum

@input_spectrum.setter
def input_spectrum(self, new_spectrum):
# We want to clear the refined locations if a new calibration spectrum is provided
self._clear_cache()
self._input_spectrum = new_spectrum

@property
def model(self):
return self._model

@model.setter
def model(self, new_model):
self._clear_cache()
self._model = new_model

@cached_property
Copy link
Member

Choose a reason for hiding this comment

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

cache needs to be cleared when appropriate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That can be accomplished by deleting the cached properties from the class __dict__, right? I haven't use cached_property before

Copy link
Member

Choose a reason for hiding this comment

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

exactly. In jdaviz I wrote a helper method that might also be useful in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I implemented what I think you had in mind, take a look.

Copy link
Member

Choose a reason for hiding this comment

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

that looks good to me, but I think there are other cases that need to clear too (wcs cache needs to clear for a change in pretty much any of the original inputs, I think). I wonder if there's a way to generalize away this logic so we don't need to have all this boilerplate code 🤔 (definitely can generalize later - it would probably take a custom __setattr__ which might be a little hacky-feeling)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also now clears the WCS cache on changing the lines or model attributes. Do you think that suffices for now? I imagine that at some point people would just make a new WavelengthCalibration1D rather than updating an existing one.

def wcs(self):
# computes and returns WCS after fitting self.model to self.refined_pixels
x = self._line_list["pixel_center"]
y = self._line_list["wavelength"]

if self.fitter is None:
# Flexible defaulting if self.fitter is None
if self.model.linear:
fitter = LinearLSQFitter(calc_uncertainties=True)
else:
fitter = LMLSQFitter(calc_uncertainties=True)
else:
fitter = self.fitter

# Fit the model
self._model = fitter(self._model, x, y)

# Build a GWCS pipeline from the fitted model
pixel_frame = cf.CoordinateFrame(1, "SPECTRAL", [0,], axes_names=["x",], unit=[u.pix,])
spectral_frame = cf.SpectralFrame(axes_names=["wavelength",],
unit=[self._line_list["wavelength"].unit,])

pipeline = [(pixel_frame, self.model), (spectral_frame, None)]

wcsobj = wcs.WCS(pipeline)

return wcsobj

def apply_to_spectrum(self, spectrum=None):
Copy link
Member

Choose a reason for hiding this comment

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

Do we want __call__ instead, to just call this, to do something else, or to not exist? I think we will eventually want calling to do something since that pattern is supported through most/all of the other specreduce operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remembered @eteq was somewhat against having a __call__ at all, so I had left it out for now. If it's more consistent with the way the rest of the package works, then I guess we can think about what makes the most sense. Having it call this method on the default (input) spectrum seems like an ok behavior to me, but I don't feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I think we'll want __call__ to do something eventually, but I'm fine deferring that until the rest of the API begins to converge in follow-up efforts.

Copy link
Contributor

Choose a reason for hiding this comment

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

a workflow we will need to support is where we iterate the fitting process. one would start with a small number of matched lines and a simple model, use the model to match more lines, re-fit, tweak model, match more lines, re-fit/tweak/match, and repeat until a convergence criteria is met. my view is that this class/function should be within that loop, but not contain that loop. i don't see where a __call__ fits into that scheme and i think a apply_to_spectrum() method is a clearer way of applying the results to a spectrum.

# returns spectrum1d with wavelength calibration applied
# actual line refinement and WCS solution should already be done so that this can
# be called on multiple science sources
spectrum = self.input_spectrum if spectrum is None else spectrum
updated_spectrum = Spectrum1D(spectrum.flux, wcs=self.wcs, mask=spectrum.mask,
uncertainty=spectrum.uncertainty)
return updated_spectrum