Synthetic wavelength calibration data #165
Conversation
…downloads for everything; add support for loading pypeit reference data; add list of pypeit-supported line lists:
…cific about arcs since sky lines are often used in lieu of or in addition to lamps
… lines if linelist is not None
…ate non-linear dispersion
specreduce/utils/synth_data.py
Outdated
| 'make_2dspec_image', | ||
| 'make_2d_arc_image' |
There was a problem hiding this comment.
should we change the function name to make_2darc_image to be consistent with the already-existing make_2dspec_image?
There was a problem hiding this comment.
how about renaming make_2dspec_image to make_2d_trace_image? that's a more accurate description of what it does. this would open up the possibility of a make_2d_spec_image that combines make_2d_trace_image and make_2d_arc_image. might be worth implementing that as part of this PR.
specreduce/utils/synth_data.py
Outdated
| wave_air : bool (default: False) | ||
| If True, convert the vacuum wavelengths used by ``pypeit`` to air wavelengths. | ||
| wave_unit : `~astropy.units.Quantity` | ||
| If ``wcs`` is not provides, this defines the wavelength units of the |
There was a problem hiding this comment.
| If ``wcs`` is not provides, this defines the wavelength units of the | |
| If ``wcs`` is not provided, this defines the wavelength units of the |
There was a problem hiding this comment.
gah, fat fingers...
specreduce/utils/synth_data.py
Outdated
| linelists : str or list of str (default: ['HeI']) | ||
| Specification for linelists to load from ``pypeit`` | ||
| amplitude_scale : float (default: 1) | ||
| Scale factor to apply to amplitudes provides in the linelists |
There was a problem hiding this comment.
| Scale factor to apply to amplitudes provides in the linelists | |
| Scale factor to apply to amplitudes provided in the linelists |
specreduce/utils/synth_data.py
Outdated
| of the dispersion axis. | ||
| wave_air : bool (default: False) | ||
| If True, convert the vacuum wavelengths used by ``pypeit`` to air wavelengths. | ||
| wave_unit : `~astropy.units.Quantity` |
There was a problem hiding this comment.
should this be a Quantity or Unit object?
There was a problem hiding this comment.
ah ha! good point. it should in fact be Unit...
| wcs = WCS(naxis=2) | ||
| wcs.wcs.ctype[0] = 'WAVE' | ||
| wcs.wcs.ctype[1] = 'PIXEL' | ||
| wcs.wcs.cunit[0] = wave_unit |
There was a problem hiding this comment.
do we need to check if the unit is valid (u.Unit and spectral)?
There was a problem hiding this comment.
i think so and i'll add that check.
| wave_air : bool (default: False) | ||
| If True, convert the vacuum wavelengths used by ``pypeit`` to air wavelengths. |
There was a problem hiding this comment.
having this between the two arguments that are only applicable if wcs is not provided might be confusing - could we move it below extent and wave_unit?
There was a problem hiding this comment.
i'll at least do that if there's not a more clever way around it...
Codecov Report
@@ Coverage Diff @@
## main #165 +/- ##
==========================================
+ Coverage 76.81% 80.20% +3.38%
==========================================
Files 9 9
Lines 716 798 +82
==========================================
+ Hits 550 640 +90
+ Misses 166 158 -8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
thanks for the review, @kecnry! i think i addressed all of the comments. i also added a test i forgot to earlier and added a few more checks to flesh out coverage. if there are no other comments/concerns, i'll go ahead and merge in the next day or two. |
| """ | ||
| Basic function to take a path to a file and load it via ``pkg_resources`` if | ||
| the ``specreduce_data`` package is available and load it via GitHub raw user content if not. | ||
| Utility to load reference data via GitHub raw user content. By default the ``specreduce_data`` |
There was a problem hiding this comment.
I thought you said specreduce-data is "deprecated" but it is still used here with no deprecation warning. Did I misunderstand?
There was a problem hiding this comment.
the use of the package is deprecated. the repository is still there and will be there unless and until something better comes along.
There was a problem hiding this comment.
OK, I opened astropy/specreduce-data#13
|
|
||
| def get_reference_file_path(path=None, cache=False, show_progress=False): | ||
|
|
||
| def get_reference_file_path( |
There was a problem hiding this comment.
Instead of rolling your own here, why not use the stuff in astropy.utils.data but use your own conf.dataurl?
https://github.com/astropy/astropy/blob/main/astropy/utils/data.py
There was a problem hiding this comment.
when this was originally implemented years ago, i followed examples in https://docs.astropy.org/en/stable/utils/data.html and just wrapped astropy.utils.data.download_file() with some error handling. it looks like those examples haven't changed much since. i'll look into refactoring this to use pure-astropy at a higher level. that's out of scope for this PR, though.
There was a problem hiding this comment.
i opened #169 to cover this. it's a little unclear to me how best to handle multiple data URLs with the conf.dataurl scheme. will play around more when i get the chance.
| @@ -8,3 +8,12 @@ def test_get_reference_file_path(): | |||
| test_path = "extinction/apoextinct.dat" | |||
| p = get_reference_file_path(path=test_path) | |||
| assert p is not None | |||
|
|
|||
|
|
|||
| def test_get_pypeit_data_path(): | |||
There was a problem hiding this comment.
If your test downloads data, you might wanna mark it with @pytest.mark.remote_data.
There was a problem hiding this comment.
good call. i'll add that.
There was a problem hiding this comment.
done. i tweaked the CI workflow so that --remote-data is only enabled for the one test where coverage is measured. this should hopefully help with the occasional failures due to downloads timing out. no longer parallel tests getting the same files at the same time...
|
Mine was just a drive-by review and not thorough. I'll leave it to specreduce maintainers to do the rest. Thanks! |
|
drive-by or not, it was useful so thanks again, @pllim! |
This PR adds a utility for creating realistic models of 2D spectroscopic images of arc lamp (or sky background) calibration observations. This will be key for testing the various stages of the wavelength calibration code and process as it's developed. Tilts/curvature along the spatial axis is modeled using 1D polynomials. Non-linear dispersion relations are modeled using the ideal disperser model implemented within the FITS WCS standard. Examples of the use of this routine are included in its docstrings.
A few other notes:
specreduce_datadependency has been deprecated. Cleaner to just always download/cache the data as needed.astropy.wcs.WCS. It would be useful to have a documented functional interface for building WCS spectrograph models that provides meaningful parameter names and units handling. However, I think that is outside of the scope of this PR. It's also an open question whether such a utility should live here or withinspecutils.