-
Notifications
You must be signed in to change notification settings - Fork 19
Adding functionality to represent non linear spectral dispersion #713
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?
Changes from 3 commits
8c43c12
cfd3d4e
2e45f28
de99978
e8cadfd
a3c8938
774fd73
5893f99
19a35c7
86d9515
7f9ba37
3dd823f
543f6cb
67e66ae
caef8c4
5913d79
f1aaae2
e49ed4f
375cf78
368cb6b
0540894
c4dc59d
58e262c
4633621
d566b1f
569b0a3
b102d5d
2fc780b
0dbf7ee
24929e6
1f307bd
2dbe78c
436a055
1f149ed
381cf27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Added dkist.wcs.models.generate_grating_spectral_transform() for building FITS -GRA/-GRI spectral transforms from header-derived grating parameters. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,9 @@ | |
|
|
||
| import astropy.modeling.models as m | ||
| import astropy.units as u | ||
| from astropy.modeling import CompoundModel, Model, Parameter, separable | ||
| from astropy.modeling import CompoundModel, Model, Parameter, custom_model, separable | ||
| from astropy.utils.decorators import deprecated_renamed_argument | ||
| from gwcs.spectroscopy import WavelengthFromGratingEquation | ||
|
|
||
|
A-Derks marked this conversation as resolved.
Outdated
|
||
| from dkist.utils.decorators import deprecated | ||
| from dkist.utils.exceptions import DKISTDeprecationWarning | ||
|
|
@@ -31,10 +32,115 @@ | |
| "VaryingCelestialTransform2D", | ||
| "VaryingCelestialTransform3D", | ||
| "generate_celestial_transform", | ||
| "generate_grating_spectral_transform", | ||
| "varying_celestial_transform_from_tables", | ||
| ] | ||
|
|
||
|
|
||
| def _grating_equation_constants( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type-hints, please |
||
| reference_wavelength, | ||
| grating_density, | ||
| spectral_order, | ||
| incident_angle, | ||
| refractive_index, | ||
| refractive_index_derivative, | ||
| out_of_plane_angle, | ||
| camera_angle, | ||
| ): | ||
| alpha = incident_angle.to(u.rad) | ||
| epsilon = out_of_plane_angle.to(u.rad) | ||
| theta = camera_angle.to(u.rad) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If these are already Quantities, then I don't think we need to convert them to radians. In this function, we only ever do trig on them, not use them directly in algebra, so the conversion isn't necessary. |
||
|
|
||
| grism_parameter = (grating_density * spectral_order) / np.cos(epsilon) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not to be That Guy, but Greisen already defines something called the "grism parameter" in Eq. 75, which is different than the definition here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "grism constant" would be an OK alternative |
||
| adjusted_incident_angle_sine = ( | ||
| refractive_index - (refractive_index_derivative * reference_wavelength).decompose() | ||
| ) * np.sin(alpha) | ||
| wavelength_parameter = grism_parameter - refractive_index_derivative * np.sin(alpha) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this variable called Based on my comment down in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Arthur, but if you do want to name these parts of the grism equation (Greisen eq 70), then I would stick with names that imply units. You already do that with |
||
| reference_refracted_angle = np.arcsin( | ||
| (grism_parameter * reference_wavelength).decompose() - refractive_index * np.sin(alpha) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the |
||
| ) | ||
| angle_slope = wavelength_parameter / ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to rename this. If we use the name "grism parameter" for Γ (i.e. rename Gm/cosϵ) then this is the slope of the grism parameter wrt wavelength and wrt the distance along the detector (ξ in the paper) scaled. Γ itself is dimensionless. I think |
||
| np.cos(reference_refracted_angle) * np.cos(theta) ** 2 | ||
| ) | ||
|
|
||
| return ( | ||
| adjusted_incident_angle_sine, | ||
| wavelength_parameter, | ||
| reference_refracted_angle, | ||
| theta, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to return True, a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eigenbrot, see my comment down on line 84. |
||
| angle_slope, | ||
| ) | ||
|
|
||
|
|
||
| def _refracted_angle_sine_model( | ||
| reference_pixel, dispersion, reference_refracted_angle, theta, angle_slope | ||
| ): | ||
| @custom_model | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you refactor this into class form, I think that with it like this it will be hard to add the required asdf schemas. |
||
| def refracted_angle_sine(pixel): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a docstring either on |
||
| wavelength_offset = ((pixel - reference_pixel) * u.pix) * dispersion | ||
| output_angle = ( | ||
| np.arctan(-np.tan(theta) + wavelength_offset * angle_slope) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renaming |
||
| + reference_refracted_angle | ||
| + theta | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is where |
||
| ) | ||
| return np.sin(output_angle) | ||
|
|
||
| return refracted_angle_sine() | ||
|
|
||
|
|
||
| def generate_grating_spectral_transform( | ||
|
A-Derks marked this conversation as resolved.
Outdated
A-Derks marked this conversation as resolved.
Outdated
|
||
| reference_pixel, | ||
| reference_wavelength, | ||
| dispersion, | ||
| grating_density, | ||
| spectral_order, | ||
| incident_angle, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. type-hints, please |
||
| refractive_index=1 * u.one, | ||
| refractive_index_derivative=0 / u.m, | ||
| out_of_plane_angle=0 * u.deg, | ||
| camera_angle=0 * u.deg, | ||
| ) -> CompoundModel: | ||
| """ | ||
| Generate a one-dimensional FITS ``-GRA``/``-GRI`` spectral transform. | ||
|
|
||
| This function handles the FITS grating/grism intermediate-angle terms and | ||
| delegates the final wavelength calculation to | ||
| ``gwcs.spectroscopy.WavelengthFromGratingEquation``. | ||
| """ | ||
|
eigenbrot marked this conversation as resolved.
|
||
| ( | ||
| adjusted_incident_angle_sine, | ||
| wavelength_parameter, | ||
| reference_refracted_angle, | ||
| theta, | ||
| angle_slope, | ||
| ) = _grating_equation_constants( | ||
| reference_wavelength=reference_wavelength, | ||
| grating_density=grating_density, | ||
| spectral_order=spectral_order, | ||
| incident_angle=incident_angle, | ||
| refractive_index=refractive_index, | ||
| refractive_index_derivative=refractive_index_derivative, | ||
| out_of_plane_angle=out_of_plane_angle, | ||
| camera_angle=camera_angle, | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears this function ( My suggestion would be to remove |
||
|
|
||
| refracted_angle_sine = _refracted_angle_sine_model( | ||
| reference_pixel=reference_pixel, | ||
| dispersion=dispersion, | ||
| reference_refracted_angle=reference_refracted_angle, | ||
| theta=theta, | ||
| angle_slope=angle_slope, | ||
| ) | ||
| alpha_in = m.Const1D(amplitude=adjusted_incident_angle_sine) | ||
| wavelength_from_grating = WavelengthFromGratingEquation( | ||
| groove_density=wavelength_parameter / spectral_order, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm reading the math right, I think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But here's the part I don't get. If groove density is just G, then where is the denominator in Greisen eq 70? I'm looking at step 3 on page 756: "Compute the wavelength \lambda from \Gamma via Eq. 70" That equation has the two numerator parts, an adjusted sin \alpha and a sin \gamma, which we pass in the return of this function. The denominator in eq 70 is what is called |
||
| spectral_order=spectral_order, | ||
| name="Spectral", | ||
| ) | ||
|
|
||
| return m.Mapping((0, 0)) | (alpha_in & refracted_angle_sine) | wavelength_from_grating | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the We might also consider referring to these angles as "input" and "output" instead of "alpha/incident" and "refracted" to be clear about the connection to the language used in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to same name convention for both |
||
|
|
||
|
|
||
|
A-Derks marked this conversation as resolved.
|
||
| def generate_celestial_transform( | ||
|
Copilot marked this conversation as resolved.
|
||
| crpix: Iterable[float] | u.Quantity, | ||
| cdelt: Iterable[float] | u.Quantity, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,10 +9,12 @@ | |
| from astropy.coordinates.matrix_utilities import rotation_matrix | ||
| from astropy.modeling import CompoundModel | ||
| from astropy.modeling.models import Tabular1D | ||
| from astropy.wcs import WCS | ||
|
|
||
| from dkist.wcs.models import (AsymmetricMapping, Ravel, Unravel, VaryingCelestialTransform, | ||
| VaryingCelestialTransform2D, VaryingCelestialTransform3D, | ||
| generate_celestial_transform, update_celestial_transform_parameters, | ||
| generate_celestial_transform, generate_grating_spectral_transform, | ||
| update_celestial_transform_parameters, | ||
| varying_celestial_transform_from_tables) | ||
|
|
||
|
|
||
|
|
@@ -52,6 +54,36 @@ def test_generate_celestial_unitless(): | |
| assert u.allclose(shift1.offset, 0) | ||
|
|
||
|
|
||
| def test_generate_grating_spectral_transform(): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add tests for just the new model in isolation? I also suspect this test should probably be in dkist-inventory and not here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in there |
||
| header = { | ||
| "CTYPE1": "AWAV-GRA", | ||
| "CUNIT1": "nm", | ||
| "CRPIX1": 218, | ||
| "CRVAL1": 854.1738582455826, | ||
| "CDELT1": 0.0022975580183395555, | ||
| "PV1_0": 23000.0, | ||
| "PV1_1": 90, | ||
| "PV1_2": 65.696, | ||
| } | ||
|
A-Derks marked this conversation as resolved.
|
||
| transform = generate_grating_spectral_transform( | ||
| reference_pixel=header["CRPIX1"] - 1, | ||
| reference_wavelength=header["CRVAL1"] * u.nm, | ||
| dispersion=header["CDELT1"] * u.nm / u.pix, | ||
| grating_density=header["PV1_0"] / u.m, | ||
| spectral_order=header["PV1_1"] * u.one, | ||
| incident_angle=header["PV1_2"] * u.deg, | ||
| ) | ||
|
|
||
| pixels = np.array([0, 100, 217, 300, 511], dtype=float) | ||
| expected = WCS(header).all_pix2world(pixels, 0)[0] * u.m | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned on the related Instead, I’d suggest using |
||
| result = transform(pixels) | ||
|
|
||
| assert isinstance(transform, CompoundModel) | ||
| np.testing.assert_allclose( | ||
| result.to_value(u.nm), expected.to_value(u.nm), rtol=1e-10, atol=1e-10 | ||
| ) | ||
|
|
||
|
|
||
| def test_update_celestial(): | ||
| trsfm = generate_celestial_transform( | ||
| crpix=[0, 0] * u.pix, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.