Harpspol

[astro-alexis]

No description provided.

[ivh]

Thanks Alexis — reviewed this against master (branch is based on the v0.9 tip, and nothing on master has touched HARPSPOL since, so no conflicts). Two of the four commits cancel each other out, and I think one of the cancellations is unintentional. Before merging I'd like your confirmation on three points.

1. The two new npz files are content-identical to the HARPS ones.

I extracted wavecal_{blue,red}_2D.npz from the branch and compared them field-by-field against pyreduce/instruments/HARPS/wavecal_{blue,red}_2D.npz:

• cs_lines identical in all 11 fields (WLC, WLL, POSC, POSM, XFIRST, XLAST, APPROX, WIDTH, HEIGHT, ORDER, flag)
• obase identical (116 blue, 89 red)
• order 0 = 3811 Å in both, i.e. bluest-first, same convention as HARPS
• only difference: the new files drop solution_2d, oincr and bad_order

Those three keys are read nowhere in PyReduce (LineList.load reads only cs_lines and obase), so as it stands the two files add ~250 kB of duplicated binary to the wheel and change nothing about wavecal.

So the ORDER -> nord-1-ORDER remap from commit 4 does not appear in the merged result — commit 1 flipped and commit 4 flipped back, or the flip was applied twice. Which means the off-by-one you found (each order carrying its one-redder neighbour's wavelengths) is not fixed by this branch. Could you check the committed files on your side? If the flip is genuinely needed, it needs to land; if the real fix turned out to be the wave_range reversal in commit 3, then the two npz copies can just be deleted and get_wavecal_filename can keep delegating to the HARPS directory the way get_mask_filename already does.

2. The scatter block removal is a silent behaviour change.

defaults/settings.json has scatter.extraction_height: 0.6 — a fraction of order separation — where the removed block had 20 px. get_expected_values still has a scatter entry, so the step stays discoverable and runnable, and would now use 0.6× the beam pitch on dual-beam orders. Either keep the settings block, or drop the scatter expectation as well so the step isn't reachable for HARPSPOL.

3. Two doc nits, easy to fix once the above is settled.

• The new get_wavecal_filename docstring says "order numbering reversed to match the top-to-bottom trace ordering (order 0 = reddest)", but order 0 is 3811 Å (the bluest), and the comment in get_wavelength_range a few lines below says blue-first. One of the two is wrong.
• Commit 3's message says the shipped npz "are still red-first and only work with the pre-0.9 ordering — run the wavecal_init step instead", which commit 4 supersedes. Squash-merging as-is gives a self-contradictory message, so please squash into one rewritten message.

Unrelated suggestion, for later: HARPSPOL ships no order_centers_{blue,red}.yaml. Since m assignment prefers order_centers over obase, shipping those would pin the order numbers directly rather than depending on a reversed wavelength_range guess — and note that guess only feeds wavecal_init, a separate path from the 2D npz.

[astro-alexis]

Thanks Thomas — I've checked all three and you're right on each. Confirmations and one addition.

1. Confirmed, and the npz files should go.

I reproduced your comparison at the PR head: cs_lines is identical to HARPS in all 11 fields for both channels, obase matches, and order 0 is the bluest (3830 Å blue, 5369 Å red). The flip from f967e5d is not in the merged result.

More usefully, I can show the flip isn't needed. The reduction I've been running for the HARPSpol archive uses exactly this state — cs_lines identical to HARPS with the wave_range reversal retained — and the wavelength solution is sound: cross-correlating 74 eps Ind sequences over six years against a median template gives a residual of 2.6 m/s median, MAD 0.6 m/s, no epoch beyond 6 m/s, and the physical echelle orders come out as blue 161–117 and red 114–89 as they should. So the real fix was the wave_range reversal in 5c7aab58, not the linelist flip.

So: delete both npz files and let get_wavecal_filename delegate to the HARPS directory the way get_mask_filename does. The diff then reduces to the settings change and comments.

One more for the squash message — ba8919c3 is titled "drop wave range reversal", but the merged result keeps wave_range = wave_range[::-1]. The rewritten message shouldn't claim it was dropped.

2. Confirmed, and worth going further than restoring the block.

I ran an A/B on a full month (17 sequences, 4 calibration nights, 2015-06), reduced both ways, and then a per-value test on one observation, HD 136504:

extraction_height	blue median I	red median I	blue orders with no usable continuum
no scatter	177828	281718	0
0.6 (inherited after removal)	−26578	42631	37
10	−30172	42631	39
20 (the removed block)	6283	67188	21

The trend is monotonic — a larger height masks more of the order, so less stellar flux leaks into the background fit — but 20 still keeps only 2.3% of the blue flux and 24% of the red, and leaves 21 orders unnormalisable. Across the full month at 0.6 the count of such orders went from 6 to 1066.

So restoring the block doesn't rescue the step; it moves it from badly broken to slightly less badly broken. Your objection stands regardless, since deleting the block while leaving the step discoverable is a silent change, but the more useful conclusion is that scatter is currently unusable for HARPSPOL at any of these settings.

The damage scales with target faintness (α Cen −3.4×, CPD-246168 −325×), which points at the model being fitted on the bright LAMP,LAMP,TUN flat and subtracted from much fainter science frames without rescaling to the exposure level. If that's right it's an upstream issue, not a settings one. Happy to open a separate issue with the numbers.

For this PR I'd take your second option: drop the scatter entry from get_expected_values as well, so the step isn't reachable for HARPSPOL until that's understood.

3. Confirmed — the docstring is the wrong one.

Order 0 is 3830 Å in blue and 5369 Å in red, both the bluest. The get_wavelength_range comment is right and the new get_wavecal_filename docstring is wrong. Will fix and squash into one message.

On order_centers_{blue,red}.yaml — agreed, and I'd like to do it. Order numbering has bitten this twice now: here, and in my own demodulation, where the routine that recovers physical echelle numbers from consecutive central wavelengths needs λc good to ~10⁻⁶ relative before its ladder collapses. Pinning the numbers explicitly removes the class of problem rather than this instance of it.

[ivh]

Agreed on all three — and thanks for the scatter A/B, that's more than I asked for. Please do open the separate issue with those numbers.

Waiting on you for: delete the two npz files and delegate get_wavecal_filename to the HARPS directory, drop the scatter entry from get_expected_values, fix the order 0 = reddest docstring, and squash into one message (which shouldn't say the wave_range reversal was dropped). Then this is good to merge.

I'll check separately whether scatter.bias_scaling / norm_scaling are actually applied in BackgroundScatter — relevant to your rescaling hypothesis.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@2a9e67a). Learn more about missing BASE report.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##             master      #38   +/-   ##
=========================================
  Coverage          ?   57.02%           
=========================================
  Files             ?       58           
  Lines             ?     8994           
  Branches          ?        0           
=========================================
  Hits              ?     5129           
  Misses            ?     3865           
  Partials          ?        0           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[astro-alexis]

Pushed the four changes — master fast-forwarded to 99a08ca, so nothing you'd already reviewed was rewritten. The PR is now 2 files, +5 −10: both npz files gone with get_wavecal_filename back to delegating to HARPS, scatter dropped from get_expected_values as well as settings.json, and the "order 0 = reddest" docstring corrected.

One correction on your point 2, from testing rather than reading: dropping the expectation does not make the step unreachable. from_instrument falls back to the flat files —

"scatter": lambda: pipe.scatter(files.get("scatter", files.get("flat"))),

— so an explicit steps=(..., "scatter", ...) still runs, on the same LAMP,LAMP,TUN frames id_scatter would have picked. Removing the declaration stops HARPSPOL advertising a step it can't use, but preventing it outright needs a guard or the scaling fix. Flagging it since making it unreachable was the stated goal.

Scatter numbers and the scaling trace are in #39 — including that bias_scaling/norm_scaling are applied by combine_calibrate to the input frames, with nothing rescaling the output between the fit and calc_scatter_correction. That may save you the look you mentioned.

Suggested squash message, since the net change is now much smaller than the branch history suggests:

HARPSPOL: disable the scatter step; document the wavelength ordering

Remove the scatter settings block and the scatter entry from
get_expected_values. Measured on 2015-06 HARPSpol data, the step removes most
or all of the stellar flux at every extraction_height tested: with the step
off the blue median intensity is 177828, against 6283 at 20 (the value this
block carried), -26578 at 0.6 and -30172 at 10. Over a full month at 0.6 the
number of orders left with no usable continuum rose from 6 to 1066. The
damage scales with target faintness, which points at the flat-derived model
being subtracted from fainter science frames without rescaling. Details and
the code trace are in #39.

Note this does not make the step unreachable: from_instrument falls back to
the flat files, `files.get("scatter", files.get("flat"))`, so an explicit
steps=(..., "scatter", ...) still runs. Removing the declaration stops
HARPSPOL advertising a step it cannot currently use; a guard or the scaling
fix would be needed to prevent it outright.

No change to the wavelength solution. get_wavecal_filename still delegates to
the HARPS npz files and get_wavelength_range still reverses the range; both
now carry comments recording why, since order 0 being the bluest (3830 A blue,
5369 A red) against a config that lists red-to-blue was the source of the
confusion this branch started from.