Wavelength template line detection by kbwestfall · Pull Request #2133 · pypeit/PypeIt

kbwestfall · 2026-05-22T00:21:09Z

I wanted to reduce the data in the RAW_DATA/keck_deimos/830G_L_8410 directory as a way of producing a test for flux calibration that would use the multi_spec_det option. Wavelength calibration on detector 3 was failing because pypeit.core.wavecal.wvutils.arc_lines_from_spec was not detecting lines in the template spectrum keck_deimos_830G.fits. This is because the FWHM is too small. I.e., the FWHM of the observed data is smaller than in the template, which is causing the lines in the template to be rejected.

This is a short term fix that adds a loop that iteratively increments the FWHM until the number of good template lines reaches some minimum fraction of all lines detected. This works for this data set, but we'll need to run the dev-suite to check how this affects other instruments.

In the longer term, I think we should

only detect the lines in the template once
or, better yet, require that the pixel and wavelength coordinates of the lines used in calibrating the template be included in the template file. I.e., we shouldn't need to re-detect the lines.

The primary changes are in pypeit.core.wavecal.wvutils.arc_lines_from_spec and pypeit.core.wavecal.autoid.reidentify. Particularly for the latter, I took the opportunity to clean-up the code and docs a bit.

profxj

Excellent addition. I might give Debora a head's up about it too
If you haven't already

tbowers7

Thanks for figuring out this issue. There is only one place that needs an actual change -- the rest are questions and comments. Approving now.

tbowers7 · 2026-06-05T18:46:22Z

+        # NOTE: I've hard-coded the values for good_frac, fwhm_incr, and
+        # max_good_iter.  We may want to revisit this during testing and/or as
+        # we continue to develop.


Is there any reason at all to expose these as user-tunable parameters, or are they strictly algorithm-specific parameters that need to be tested and set for good?

My hope is that these are actually (somewhat) temporary parameters, so I think the answer to your first question is no. Particularly because it's would be difficult to add a way for a user to assess what the parameters should be without digging into the code.

tbowers7 · 2026-06-05T18:57:03Z

+            f'shift = {shift_vec[iarxiv]:5.3f}, stretch = {stretch_vec[iarxiv]:5.3f}, cc = '
+            f'{ccorr_vec[iarxiv]:5.3f}'


Formatting annoyance: place the "cc =" on the next line -- doesn't affect anything other than me reading the code.

tbowers7 · 2026-06-05T19:09:47Z

        # Grab the observed arc spectrum
        obs_spec_i = spec[:,slit]
        # get FWHM for this slit
+        # TODO: Doesn't this mean measured_fwhms *cannot* be None?


That's how I read it. Maybe add a defensive check for None?

Instead of checking for None, I made it a positional argument.

tbowers7 · 2026-06-05T19:10:45Z

+
+            # TODO: JFH This continue statement deals with the case when the
+            # msnippet derives from *entirely* zero-padded pixels, and allows
+            # the code to continue with crashing. This code is constantly


without crashing?

Good catch! Fixed.

tbowers7 · 2026-06-05T20:40:37Z

+    if all(iter_args) and len(w)/len(tampl) < good_frac:
+        # Iteratively increase the FWHM in an attempt to improve the line detection
+        for i in range(max_good_iter):
+            _fwhm = fwhm*fwhm_incr**(i+1)


So, if fwhm_incr were less than 1, we would be decreasing the FWHM with each iteration. Is there a case where this would be useful at all?

In the limited testing I've done, it seems like you tend to miss lines if the FWHM is too small, but not if it is too big. That means there is not much to be gained by iteratively making the FWHM smaller. But, I didn't think it was worth imposing that fwhm_incr be greater than 1, particularly given that it's effectively a developer-only parameter in the current workflow. I've added a comment along these lines.

tbowers7 · 2026-06-05T20:42:40Z

-                zerolag_shift_stretch, args=(y1,y2), x0=x0_guess, tol=toler, 
-                bounds=bounds, disp=False, polish=True, seed=seed)
+            zerolag_shift_stretch, args=(y1,y2), x0=x0_guess, tol=toler, bounds=bounds, disp=False,
+            polish=True, rng=seed


seed = rng instead?

The seed keyword is being deprecated by scipy, in favor of rng.

tbowers7 · 2026-06-05T20:47:06Z

                continue
            # Detect lines and store the spectral FWHM
            _, _, cent, wdth, _, best, _, nsig = arc.detect_lines(arc_spec.squeeze(), sigdetect=sigdetect, fwhm=fwhm, bpm=arc_spec_bpm.squeeze())
+            # NOTE: "best" is a list of line indices, NOT a boolean array!


This kind of NOTE occurs often in this PR...

This indicates that a boolean array is expected for this kind of variable... is there a good reason it is not and/or would it be prohibitively expensive (code changes / in-process computation / etc.) to make it thus?

Personally, I often expect that these kinds of arrays are boolean. So I added these (admittedly obnoxious) notes mostly as a reminder to me. At the same time, I didn't think it was worth actually trying to change the object from an int array to a bool array. Punting...

kbwestfall

Thanks for the reviews! All comments addressed. Next is to run the tests!

kbwestfall · 2026-06-12T16:08:57Z

+        # NOTE: I've hard-coded the values for good_frac, fwhm_incr, and
+        # max_good_iter.  We may want to revisit this during testing and/or as
+        # we continue to develop.


My hope is that these are actually (somewhat) temporary parameters, so I think the answer to your first question is no. Particularly because it's would be difficult to add a way for a user to assess what the parameters should be without digging into the code.

kbwestfall · 2026-06-12T16:10:02Z

+            f'shift = {shift_vec[iarxiv]:5.3f}, stretch = {stretch_vec[iarxiv]:5.3f}, cc = '
+            f'{ccorr_vec[iarxiv]:5.3f}'


kbwestfall · 2026-06-12T16:13:31Z

                continue
            # Detect lines and store the spectral FWHM
            _, _, cent, wdth, _, best, _, nsig = arc.detect_lines(arc_spec.squeeze(), sigdetect=sigdetect, fwhm=fwhm, bpm=arc_spec_bpm.squeeze())
+            # NOTE: "best" is a list of line indices, NOT a boolean array!


Personally, I often expect that these kinds of arrays are boolean. So I added these (admittedly obnoxious) notes mostly as a reminder to me. At the same time, I didn't think it was worth actually trying to change the object from an int array to a bool array. Punting...

kbwestfall · 2026-06-12T16:20:53Z

        # Grab the observed arc spectrum
        obs_spec_i = spec[:,slit]
        # get FWHM for this slit
+        # TODO: Doesn't this mean measured_fwhms *cannot* be None?


Instead of checking for None, I made it a positional argument.

kbwestfall · 2026-06-12T16:21:55Z

+
+            # TODO: JFH This continue statement deals with the case when the
+            # msnippet derives from *entirely* zero-padded pixels, and allows
+            # the code to continue with crashing. This code is constantly


Good catch! Fixed.

kbwestfall · 2026-06-12T16:27:33Z

+    if all(iter_args) and len(w)/len(tampl) < good_frac:
+        # Iteratively increase the FWHM in an attempt to improve the line detection
+        for i in range(max_good_iter):
+            _fwhm = fwhm*fwhm_incr**(i+1)


In the limited testing I've done, it seems like you tend to miss lines if the FWHM is too small, but not if it is too big. That means there is not much to be gained by iteratively making the FWHM smaller. But, I didn't think it was worth imposing that fwhm_incr be greater than 1, particularly given that it's effectively a developer-only parameter in the current workflow. I've added a comment along these lines.

kbwestfall · 2026-06-12T16:29:52Z

-                zerolag_shift_stretch, args=(y1,y2), x0=x0_guess, tol=toler, 
-                bounds=bounds, disp=False, polish=True, seed=seed)
+            zerolag_shift_stretch, args=(y1,y2), x0=x0_guess, tol=toler, bounds=bounds, disp=False,
+            polish=True, rng=seed


The seed keyword is being deprecated by scipy, in favor of rng.

kbwestfall added 3 commits May 21, 2026 16:51

iterative line detection

3694e49

clean up

7bb4c69

comment on hard-coded arguments

16026f4

kbwestfall added the bug Bug fix. label May 22, 2026

kbwestfall requested review from profxj and tbowers7 May 22, 2026 00:25

kbwestfall added 2 commits May 22, 2026 10:15

Merge branch 'develop' into arxiv_line_detect

1fdf19c

doc update and release note

cf7baf6

profxj approved these changes May 22, 2026

View reviewed changes

Comment thread pypeit/core/wavecal/autoid.py

Comment thread pypeit/core/wavecal/autoid.py

tbowers7 added this to the v2.1.0 milestone Jun 1, 2026

tbowers7 approved these changes Jun 5, 2026

View reviewed changes

kbwestfall added 2 commits June 12, 2026 07:35

Merge branch 'develop' into arxiv_line_detect

c39011d

PR comments

4f98f54

kbwestfall commented Jun 12, 2026

View reviewed changes

kbwestfall added 9 commits June 16, 2026 09:25

Merge branch 'develop' into arxiv_line_detect

e6becfb

testing

250c123

testing

4d324cc

testing

50f11fd

Merge branch 'develop' into arxiv_line_detect

aee8aa4

finished testing

26543cf

xcorr function clean-up

7aa02ab

Merge branch 'develop' into arxiv_line_detect

63acdee

Merge branch 'develop' into arxiv_line_detect

b6ac935

		f'shift = {shift_vec[iarxiv]:5.3f}, stretch = {stretch_vec[iarxiv]:5.3f}, cc = '
		f'{ccorr_vec[iarxiv]:5.3f}'

Uh oh!

Conversation

kbwestfall commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

profxj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tbowers7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbwestfall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kbwestfall commented May 22, 2026 •

edited

Loading