Skip to content

Fix CTD index subsets that select fewer observations - #2382

Merged
richardsc merged 1 commit into
dankelley:developfrom
dajiaohuang:fix/ctd-subset-indices-20261016
Oct 18, 2026
Merged

richardsc merged 1 commit into
dankelley:developfrom
dajiaohuang:fix/ctd-subset-indices-20261016

Conversation

@dajiaohuang

Copy link
Copy Markdown
Contributor

Problem

The documented subset(ctd, indices = ...) path compares the length of every data/flag vector with the number of selected indices, rather than the number of observations in the input. Selecting two rows from a five-row CTD therefore silently returns all five rows (and their original flags).

library(oce)
x <- as.ctd(c(34,34.2,34.4,34.6,34.8), c(12,11,10,9,8), c(0,5,10,15,20))
subset(x, indices=c(1L,3L))[["pressure"]]
# before: 0 5 10 15 20
# after:  0 10

Change

Use the input temperature-vector length to identify observation-length data and flags. This retains the existing treatment of uncommon-length items from #2250. Add synthetic public-API regression coverage for shorter, single-row, reordered, negative and empty selections, flags and station metadata.

Validation

  • R 4.3.3 / Ubuntu 24.04: source build, install and load passed.
  • The public-API reproduction failed on develop 131d14e99fe5ad3fbce68823ca33a28291fffe25 and passes with this change; new testthat file passes.
  • Synthetic CTD conversion, logical subset, metadata, a small synthetic SBE CNV read, selected numeric APIs, headless PDF profile plotting and the reviewed synthetic as.ctd example passed.
  • R CMD check --no-tests --no-examples --no-vignettes --no-manual: completed with 2 warnings (unbuilt vignettes) and 3 notes (unavailable suggests/xrefs and installed size). Full upstream tests/examples and vignette execution were not run; no private observation files, external data services or instruments were used.

@richardsc

Copy link
Copy Markdown
Collaborator

Tests pass, I'll merge.

@richardsc
richardsc merged commit 29719c8 into dankelley:develop Oct 18, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants