Initial implementation of line matching #182

tepickering · 2023-06-21T13:47:39Z

This PR adds functions to find and centroid lines in a calibration, e.g. arc lamp, spectrum and then match pixel positions to wavelengths using an input WCS. More work is needed to close the loop between this and the fitting process for wavelength calibration, but this is a start.

This PR also contains come code cleanups and the addition of typing in several places. To take advantage of the significant improvements in typing and type handling in newer versions of python, the minimum python version has been upped to 3.10. The CI and tox configuration has been updated to reflect this.

…d-coding Moffat1D. need Gaussian1D for some test cases

…ct that types/defaults are now self-documented by the code

…es to >= 3.10

…input as np.array which needs to happen, anyway

codecov · 2023-06-21T13:52:33Z

Codecov Report

Attention: Patch coverage is 92.92929% with 7 lines in your changes missing coverage. Please review.

Project coverage is 83.37%. Comparing base (aea9d50) to head (1845c6f).
Report is 17 commits behind head on main.

Files	Patch %	Lines
specreduce/line_matching.py	91.83%	4 Missing ⚠️
specreduce/__init__.py	50.00%	2 Missing ⚠️
specreduce/calibration_data.py	97.61%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   81.91%   83.37%   +1.45%     
==========================================
  Files          10       13       +3     
  Lines        1001     1137     +136     
==========================================
+ Hits          820      948     +128     
- Misses        181      189       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ible

specreduce/line_matching.py

specreduce/tests/test_linelists.py

specreduce/utils/synth_data.py

specreduce/line_matching.py

specreduce/tests/test_line_matching.py

tepickering · 2023-12-15T01:42:48Z

i think i successfully merged in the significant changes from #202. the python 3.8 and 3.9 tests did fail as expected so i bumped requires-python to 3.10 as we agreed to initially. i tweaked tox.ini and the workflows accordingly to get the tests to all pass.

tepickering · 2024-02-08T22:36:12Z

this PR has been languishing for a while. i'd like to finally get this in to at least add the type annotation work and up the minimum python to 3.10. there's more work to be done on the line matching and especially wavelength calibration, but at this point it's better off done in new PRs.

cshanahan1

still testing out the functionality, but some initial comments on code.

I noticed that throughout, you've removed some of the numpy style docstrings that describe input parameter type and default, and are instead annotating the function. I think that the annotations are a helpful addition, but that the docstrings should retain the same format as before, making sure defaults are also described.

cshanahan1 · 2024-03-07T04:55:05Z

specreduce/__init__.py

 from specreduce.core import *  # noqa
 from specreduce.wavelength_calibration import * # noqa
+


is this necessary? specreduce.__version__ seems to be correct already

it may not be. i just moved it over from _astropy_init.py as-is so there'd be no surprises. kind of a vestige of the old astropy-helpers, but other coordinated packages like photutils have this in their __init__.py as well, fwiw.

specreduce/line_matching.py

cshanahan1 · 2024-03-07T05:49:13Z

specreduce/line_matching.py

+    # Extra sanity handling to make sure the input Sequence can be converted to an np.array
+    try:
+        pixel_positions = np.array(pixel_positions, dtype=float)
+    except ValueError as e:


add a small test for this case to raise the error. Also, as part of sanity checking, maybe add in a check that wcs exists and its spectral.

cshanahan1 · 2024-03-07T15:18:22Z

specreduce/line_matching.py

+    widths = []
+    amplitudes = []
+    for r in detected_lines:
+        g_init = models.Gaussian1D(


I think this will cause an error when the model is evaluated unless stddev, mean, and amp are all in the same unit. Stddev is in pixels but it looks like amplitude will have the same unit as flux?

it's units-aware and is ultimately getting the unit information from the Spectrum1D instance. i was, however, incorrect to assume fwhm should be pixels if not specified. it should be in whatever the spectral_axis unit is in the input spectrum.

cshanahan1 · 2024-03-07T21:40:59Z

specreduce/calibration_data.py

@@ -88,44 +90,47 @@
 ]


+def get_available_line_catalogs() -> dict:


I would maybe add a call to this to one of the existing tests (like in test_line_matching.py) to make sure this function is covered in tests.

cshanahan1 · 2024-03-07T21:48:02Z

specreduce/calibration_data.py

@@ -2,21 +2,23 @@
 Utilities for defining, loading, and handling spectroscopic calibration data
 """

-import os
 import warnings


sort import alphabetically (move warnings import down, and astropy.coordinates up)

cshanahan1 · 2024-03-07T22:13:32Z

specreduce/calibration_data.py

+        'pypeit': PYPEIT_CALIBRATION_LINELISTS
+    }
+
+
 def get_reference_file_path(


I'm a bit confused by this function. If you don't give it a path, it quietly does nothing. It's also not clear if something is being overwritten or not. Also, it seems to only allow you to download something to a .specreduce directory (which it creates), or if you set cache to False it puts it in /var/. Would it make sense to allow specifying an output directory? Probably out of the scope of this, but I had never used this before reviewing this PR and it stood out to me as confusing.

cshanahan1 · 2024-03-08T06:02:49Z

specreduce/line_matching.py

+
+    catalog_pixels = spectral_wcs.world_to_pixel(catalog_wavelengths)
+    separations = pixel_positions[:, np.newaxis] - catalog_pixels
+    matched_loc = np.where(np.abs(separations) < tolerance)


maybe raise a warning when there are no matches? it might be more informative than just returning an empty table

cshanahan1 · 2024-03-08T06:07:50Z

specreduce/tests/test_line_matching.py

@@ -0,0 +1,110 @@
+import pytest


alphabetize imports

cshanahan1 · 2024-03-08T06:08:42Z

specreduce/tests/test_line_matching.py

+        'CRVAL2': 0,           # Reference value
+        'CDELT2': 1            # Spatial units per pixel
+    }
+    non_linear_wcs = WCS(header=non_linear_header)


does it make sense to also test this with GWCS since the function can accept either? im not sure if thats overkill

i am working on a draft PR to formally add gwcs as a dependency (it is currently an import in one place, but not a defined dependency in pyproject.toml), add tests to cover its use in addition to astropy.wcs, and add better examples of its use in contexts like this. so, yes, it definitely makes sense, but it's out of scope for this PR.

tepickering · 2024-03-08T06:49:47Z

still testing out the functionality, but some initial comments on code.

I noticed that throughout, you've removed some of the numpy style docstrings that describe input parameter type and default, and are instead annotating the function. I think that the annotations are a helpful addition, but that the docstrings should retain the same format as before, making sure defaults are also described.

this was a very intentional change. now that type annotations are supported, they should be the preferred way to document typing and defaults. it is a much more flexible and powerful way of doing so. e.g., it empowers compilers to optimize the code using the given information which is important in contexts such as GPUs. what we definitely DO NOT WANT is to have redundant information in two places because it is guaranteed to create inconsistencies in the future.

cshanahan1 · 2024-03-08T17:31:25Z

still testing out the functionality, but some initial comments on code.
I noticed that throughout, you've removed some of the numpy style docstrings that describe input parameter type and default, and are instead annotating the function. I think that the annotations are a helpful addition, but that the docstrings should retain the same format as before, making sure defaults are also described.

this was a very intentional change. now that type annotations are supported, they should be the preferred way to document typing and defaults. it is a much more flexible and powerful way of doing so. e.g., it empowers compilers to optimize the code using the given information which is important in contexts such as GPUs. what we definitely DO NOT WANT is to have redundant information in two places because it is guaranteed to create inconsistencies in the future.

That makes sense not to keep in in two places. So this is the new best practice then? If so maybe we can make a PR to do this across the whole package so its not in the diff here, that should be fairly quick to do

cshanahan1

Tests are passing so I'm going to merge this, thanks!

tepickering added 30 commits April 12, 2023 16:23

rename and move function to return available line catalogs.

c7c11d5

tweak docstring; add initial sketch for LineMatch classes

eb002d4

dev notebook work; more line_matching sketch-up

9130a1b

add ruff config to pyproject for faster linting

2f3714d

remove check for astropy < 3.x

d035f85

add missing function to __all__ in synth_data.py

7f7e350

Merge remote-tracking branch 'upstream/main' into line_matching

dc641db

make the spatial profile for a traced source a passed argument vs har…

a63aa45

…d-coding Moffat1D. need Gaussian1D for some test cases

codestyle cleanups

aad31d4

flesh out type annotations in synth_data and edit docstrings to refle…

026a600

…ct that types/defaults are now self-documented by the code

notebook edits

608e111

comment out undefined method for now

3ad3b4f

change relative to absolute imports

60b624c

remove astropy_helpers vestige; packaging cleanups; set python_requir…

6f1a7aa

…es to >= 3.10

update tox.ini to reflect new python_requires

f6e50d0

update CI workflows

4f4d660

flesh out more type annotations

96c52ad

add support for comma-separated strings to build line catalogs

cd9717d

update dev notebook to reflect some api changes in linelists

ef57d34

flesh out match_lines_wcs; save notebook dev

be406a7

actually commit dev notebook

d61cd63

loosen typing on match_lines_wcs, but add sanity checking by casting …

6b4f9f6

…input as np.array which needs to happen, anyway

add routine to find arc lines and centroid them

9d27c7e

fix units in match_lines_wcs

8eeb8e0

allow fwhm to be a float and assume u.pix if so

f68267e

fix line matching tests to ignore warnings and mark use of remote data

79e31ea

add wavecal_demo notebook

6b6f75a

Merge remote-tracking branch 'upstream/main' into line_matching

7f7a192

fix some post-merge issues

c098278

fix column name to match

4bd0652

demo notebook updates; codestyle fixes

1e4577c

tepickering added 2 commits June 21, 2023 16:13

remove old numpy versions from tox.ini that aren't python 3.10 compat…

10ef0a1

…ible

tweak up min tested scipy version

3a1d1ac

cshanahan1 reviewed Jul 17, 2023

View reviewed changes

tepickering added 4 commits November 1, 2023 16:28

ignore new datetime warnings in python 3.12

298a4fe

Merge branch 'main' into line_matching

553d6f2

remove crutches for py312; don't seem to be needed anymore

8c8123d

fix typo; clarify docstring; simplify check

e310f07

tepickering mentioned this pull request Dec 7, 2023

MNT: Infrastructure and other updates #202

Merged

tepickering added 3 commits December 14, 2023 18:14

resolve the myriad conflicts encountered when merging astropy#202

1436ce0

codestyle fix

c6a8bc8

code now is using features that require python >= 3.10

8994bb9

cshanahan1 reviewed Mar 8, 2024

View reviewed changes

tepickering added 3 commits May 17, 2024 14:55

add warning to line_matching.py if the matched set is empty

f226188

fix codestyle bug; remove mark from fixture in test_line_matching

1ed3c2c

fix name in workflow

1845c6f

cshanahan1 approved these changes May 17, 2024

View reviewed changes

cshanahan1 merged commit 5e4439a into astropy:main May 17, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of line matching #182

Initial implementation of line matching #182

tepickering commented Jun 21, 2023

codecov bot commented Jun 21, 2023 •

edited

Loading

tepickering commented Dec 15, 2023

tepickering commented Feb 8, 2024

cshanahan1 left a comment

cshanahan1 Mar 7, 2024

tepickering May 14, 2024 •

edited

Loading

cshanahan1 Mar 7, 2024

cshanahan1 Mar 7, 2024

tepickering May 14, 2024

cshanahan1 Mar 7, 2024

cshanahan1 Mar 7, 2024

cshanahan1 Mar 7, 2024

cshanahan1 Mar 8, 2024

cshanahan1 Mar 8, 2024

cshanahan1 Mar 8, 2024

tepickering Mar 8, 2024

tepickering commented Mar 8, 2024

cshanahan1 commented Mar 8, 2024

cshanahan1 left a comment

		from specreduce.core import * # noqa
		from specreduce.wavelength_calibration import * # noqa

		@@ -88,44 +90,47 @@
		]


		def get_available_line_catalogs() -> dict:

Initial implementation of line matching #182

Initial implementation of line matching #182

Conversation

tepickering commented Jun 21, 2023

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

tepickering commented Dec 15, 2023

tepickering commented Feb 8, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tepickering May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tepickering commented Mar 8, 2024

cshanahan1 commented Mar 8, 2024

cshanahan1 left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2023 •

edited

Loading

tepickering May 14, 2024 •

edited

Loading