[codex] Add peak properties step for FWHM and prominence

[JamesHWade]

What changed

Adds a new step_measure_peaks_properties() recipe step that computes derived peak metrics from detected peaks, currently fwhm and prominence, and stores them on the .peaks tibble.

This change also:

• wires step_measure_peaks_filter(min_prominence = ...) to use the computed prominence column instead of ignoring that argument
• allows step_measure_peaks_to_table() to export computed properties like peak_1_fwhm and peak_1_prominence
• updates roxygen docs and pkgdown reference indexing for the new step

Why it changed

Issue #78 asked for full width at half maximum calculations, and noted that peak prominence would also be useful. The package already had peak detection and integration, but no supported step for deriving and exporting those peak metrics.

User impact

Users can now add a peak-properties step into an existing peak workflow, for example after step_measure_peaks_detect(), and then filter or export FWHM/prominence values in downstream analysis.

Root cause

Peak workflows exposed peak boundaries and areas, but there was no public API to calculate secondary peak descriptors from the observed signal. In addition, min_prominence existed on the filter step without corresponding property computation.

Validation

• air format .
• jarl check . --fix --allow-dirty
• R -q -e 'devtools::test(filter = "peak-operations")'
• R -q -e 'devtools::check()'
• R -q -e 'pkgdown::build_site()'

Closes #78

[Copilot]

Pull request overview

Adds a new peak-metrics recipe step to compute derived properties (currently prominence and FWHM) on detected peaks, and then enables downstream filtering/export of those computed properties within the peak workflow.

Changes:

• Introduces step_measure_peaks_properties() to compute prominence and fwhm columns on the .peaks tibble.
• Updates step_measure_peaks_filter(min_prominence=...) to actually filter using the computed prominence column (and error if it’s missing).
• Updates tests and documentation/pkgdown/NAMESPACE to cover and expose the new step.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/testthat/test-peak-operations.R	Adds Gaussian-based tests for computed prominence/FWHM, plus coverage for prominence filtering and table export.
R/peak-operations.R	Implements step_measure_peaks_properties() and helpers; wires min_prominence filtering to the prominence column.
NAMESPACE	Exports the new step and registers S3 methods (prep/bake/print/tidy).
man/tidy.recipe.Rd	Adds tidy method alias entry for the new step.
man/step_measure_peaks_to_table.Rd	Adds new step to “Other peak-operations” cross-links.
man/step_measure_peaks_properties.Rd	New Rd page for step_measure_peaks_properties().
man/step_measure_peaks_integrate.Rd	Adds cross-link to the new properties step.
man/step_measure_peaks_filter.Rd	Documents that min_prominence requires a prominence column (from the properties step).
man/step_measure_peaks_detect.Rd	Adds documentation pointer to compute additional peak metrics via the new step.
man/step_measure_peaks_deconvolve.Rd	Adds cross-link to the new properties step.
_pkgdown.yml	Adds the new step to the pkgdown reference index.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In .calculate_peak_fwhm(), peak_idx/peak_height are taken from the detected peak_location. After subtracting the local linear baseline, the true maximum of corrected_y can shift (e.g., with a sloped background), which will bias the half-height target and can yield an incorrect FWHM. Consider locating the peak as which.max(corrected_y) (and using that index for the left/right crossing searches) so FWHM is computed around the baseline-corrected peak maximum rather than the original detected position.

[Copilot]

Now that min_prominence is actually enforced, consider validating it (numeric, length 1, finite, and non-negative) when the step is created or prepped, similar to the validation done in step_measure_peaks_detect(). Without this, non-numeric values can surface later as less-informative comparison errors during bake().

[JamesHWade]

@claude review this PR

[claude]

Claude finished @JamesHWade's task in 3m 15s —— View job

---

PR Review: Add peak properties step for FWHM and prominence

• Gather context and understand the request
• Read implementation in R/peak-operations.R
• Read test file changes
• Review documentation and pkgdown changes
• Post comprehensive review

---

Overall this is a clean, well-structured addition. The FWHM and prominence calculations are mathematically sound for the common case, and the recipe step pattern is implemented correctly. A few issues worth addressing before merge:

---

Bug: test for min_prominence error fires at wrong lifecycle stage

tests/testthat/test-peak-operations.R (new test ~line 375):

test_that("step_measure_peaks_filter min_prominence requires prominence column", {
  expect_error(
    ... |>
      step_measure_peaks_filter(min_prominence = 0.4) |>
      prep(),     # <-- error expected here
    "Use .*step_measure_peaks_properties"
  )
})

The error message lives in bake.step_measure_peaks_filter, not in prep.step_measure_peaks_filter. prep() calls prep.step_measure_peaks_filter(), which only checks that a .peaks column exists — it does not apply the filter. The actual cli_abort() fires inside bake(). If this test is currently passing it is either passing for the wrong reason or there is a different error being thrown. The fix is:

# build and prep first (no error expected)
rec <- recipe(~., data = create_peaked_data()) |>
  ...
  step_measure_peaks_filter(min_prominence = 0.4) |>
  prep()

# error should surface at bake time
expect_error(
  bake(rec, new_data = NULL),
  "Use .*step_measure_peaks_properties"
)

Fix this →

---

Issue: min_prominence not validated at construction time in step_measure_peaks_filter

R/peak-operations.R around line 1329 (step_measure_peaks_filter):

The function accepts min_prominence = NULL with no validation. In contrast, step_measure_peaks_detect() validates min_prominence immediately (lines 457–461):

if (
  !is.numeric(min_prominence) ||
    length(min_prominence) != 1 ||
    min_prominence < 0
) {
  cli::cli_abort("{.arg min_prominence} must be a non-negative number.")
}

Since min_prominence is now actually enforced in bake(), passing a non-numeric or negative value will surface an obscure comparison error rather than a clear message. Adding the same guard to step_measure_peaks_filter() would be consistent and user-friendly:

if (!is.null(min_prominence)) {
  if (!is.numeric(min_prominence) || length(min_prominence) != 1 ||
        !is.finite(min_prominence) || min_prominence < 0) {
    cli::cli_abort("{.arg min_prominence} must be a non-negative number.")
  }
}

Fix this →

---

Minor: silent column clamping in bake.step_measure_peaks_properties

R/peak-operations.R around line 1238:

measure_col <- measure_cols[min(i, length(measure_cols))]

If there are more peaks columns than measure columns this silently reuses the last measure column. In practice this is unlikely, but a mismatch would produce incorrect results with no warning. The more defensive pattern — consistent with how bake.step_measure_peaks_integrate handles this — would be to check lengths match or use explicit 1-to-1 pairing with an informative error.

---

Minor: which.min(abs(...)) vs which.max(corrected_y) for FWHM peak centre (Copilot comment)

R/peak-operations.R line 1156:

peak_idx <- which.min(abs(region_x - peak_location))
peak_height <- corrected_y[peak_idx]

Because peak_location is explicitly injected into region_x (line 1141–1146), which.min(abs(...)) reliably lands on peak_location, so peak_height is the baseline-corrected height at the detected peak position. The Copilot suggestion (which.max(corrected_y)) would shift the reference point to the true post-correction maximum, which is more correct for peaks on a steep baseline slope. For typical spectroscopy use-cases the difference is negligible, but it is worth noting as a future refinement if users report slightly-off FWHM values on sloped backgrounds.

---

Looks good

• The mathematical definitions (baseline-corrected FWHM, prominence as peak_y - max(left_base_y, right_base_y)) are consistent with the internal prominence algorithm in .detect_peaks_prominence() (line 262–263).
• Linear interpolation for half-max crossings (.interpolate_crossing) is the correct approach for discretely-sampled spectra.
• Edge case handling (NA_real_ returns for degenerate inputs) is thorough.
• The recipe plumbing (prep / bake / tidy / print) follows package conventions correctly.
• Docs and _pkgdown.yml registration are complete.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e9739ee78

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Support descending axes in FWHM calculation

.calculate_peak_fwhm() treats left_base >= right_base as invalid, but for monotonic decreasing axes (common in spectroscopy, e.g. 4000→400 cm⁻¹) detected peaks naturally have left_base numerically greater than right_base. In that case this early return makes FWHM NA for otherwise valid peaks, so step_measure_peaks_properties("fwhm") cannot produce usable widths on descending-axis data. The region extraction should use the boundary range (or index positions) rather than assuming ascending axis values.

Useful? React with 👍 / 👎.