Skip to content

Reject invalid and conflicting validate truth rows - #660

Open
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/return-validate-truth-bed-errors
Open

Reject invalid and conflicting validate truth rows#660
SuhasSrinivasan wants to merge 2 commits into
nanoporetech:masterfrom
SuhasSrinivasan:codex/return-validate-truth-bed-errors

Conversation

@SuhasSrinivasan

Copy link
Copy Markdown
Contributor

Fixes #658.

Summary

  • Return every validate ground-truth BED open, read, syntax, and semantic error with file and physical-line context before output creation.
  • Reject conflicting valid labels at one (contig, strand, position) while preserving idempotent overlaps and strand independence.
  • Document the existing exact +/- truth-strand contract and add sentinel-preservation regressions for invalid inputs.

Severity

Severity: High — scientific correctness and output integrity

Rationale: Malformed truth rows can be silently discarded and conflicting annotations can change full contingency counts by row order while validate exits successfully. Existing output can also be truncated before a later truth-input failure.

Root cause

The truth parser used filter_map(... .ok()), converting every line read or parse error into an omitted row. The line parser did not enforce nonnegative nonempty intervals, consumed only the first strand character, and accepted an overly broad modification-token set. Parsed positions were inserted with unconditional last-write-wins semantics, and the report file was created before truth parsing completed.

Implementation

  • Replace the lossy iterator with an explicit enumerated line loop that preserves physical line numbers and returns contextual open/read/parse errors.
  • Ignore blank and leading-whitespace comment rows anywhere while validating every other row completely.
  • Enforce 0 <= start < end, exact +/- strands, and a documented truth-label grammar: canonical -, numeric ChEBI, one lowercase ASCII short code, or supported one-character uppercase base code.
  • Preserve numeric-first parsing so one-digit identifiers remain ChEBI codes.
  • Use BTreeMap::entry plus interval-level provenance to reject conflicting assignments with both old/new provenance; identical duplicates remain idempotent and different strands independent.
  • Keep positions as a Range<i64> rather than allocating a temporary vector.
  • Defer output creation until all truth BEDs parse and the canonical base is derived.
  • Update the validate guide to state the exact strand contract.

The two commits separate strict input/error handling from conflict detection.

Preserved behavior

  • Blank lines, comments, documented modification labels, canonical -, numeric ChEBI codes, and extra BED columns remain accepted.
  • Identical duplicate and partially overlapping assignments are idempotent.
  • The same coordinate on + and - remains independent.
  • Valid existing validate output is unchanged.

Non-goals

  • No unstranded (.) truth mode and no special UCSC track/browser directive support are introduced.
  • Observation accounting, thresholds, balancing, BAM iteration, and output schema are unchanged.
  • This PR does not alter the separate multi-truth-reader or fallback-classification defects.

Behavior before and after

Case Before After Expected oracle
Valid row followed by not-a-bed-row Invalid row silently dropped; exit 0; report overwritten Nonzero error with path/line 2/reason; report untouched No accepted row is silently lost
Negative, reversed, or zero-width interval Accepted or silently mishandled Rejected with exact coordinate reason 0 <= start < end
Strand +junk or malformed modification label Prefix or broad token may be accepted Rejected with exact token reason Complete-token parsing
Overlapping m then h versus h then m Last row wins; full scientific table changes by order Both orders fail with old/new provenance Conflicting truth is invalid, independent of order
Identical overlap or same coordinate on opposite strands Accepted Accepted Idempotence and strand independence
Invalid input with pre-existing report Report truncated before error Sentinel preserved byte-for-byte Validate inputs before output creation

Testing

Test environment

  • Revision tested: 585a9791b711e2bd2fb18cd9ed08bdad131b3f14
  • Tree tested and worktree state: 01fd7f3086e390f61e056a67eb172e2986bffdbd; clean
  • Parent revision: 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c
  • Toolchain: rustc 1.90.0; cargo 1.90.0
  • Platform: macOS 26.6, arm64
  • Reference binary: installed modkit 0.6.4
  • Reference BAM: repository fixture tests/resources/input_5mC.bam
  • Dependency resolution identity: ignored test-only Cargo.lock SHA-256 49c08c4c51b6f4320726551146d971fa9ef2183d40c3f6c631b2005965e242c0; resolved hts-sys 2.2.0; the lockfile is not in this diff
Test layer Exact command, fixture, or matrix Result and evidence
Core: parent-red regression The malformed and conflict-order commands in the linked issue on installed modkit 0.6.4 All three commands exit 0; malformed line is dropped and sentinel overwritten; conflict order changes the full table
Core: focused regression cargo test -p mod_kit validate::subcommand::tests -- --test-threads=1 8 passed, 0 failed, 0 ignored at the exact head
Core: affected CLI/integration tests cargo test -p modkit --test test_validate -- --test-threads=1 3 passed, 0 failed, 0 ignored; the invalid-input test covers five CLI failure/sentinel cases
Core: applicable full workspace gate cargo test --workspace --all-targets -- --test-threads=1 on the frozen exact head Every active workspace/all-target test passed with 0 failures
Installed-versus-fixed comparison malformed-truth.bed, conflict-mh.bed, and conflict-hm.bed from the linked issue Installed exits 0 for all; fixed exits 1 with path/line/root cause for malformed input and path/line/coordinate/strand/old/new provenance for both conflict orders
Input-token matrix Negative/reversed/zero-width intervals; strand suffix; punctuation/multi-letter/uppercase/non-ASCII invalid codes; documented short codes; ChEBI 1 and 21839; canonical -; extra columns Invalid cases fail on physical line 4 with exact reasons; valid cases parse to exact expected labels
Overlap matrix Both conflict orders; later conflict after identical overlap; exact/partial identical overlap; opposite strands Conflicts fail deterministically; first-assignment provenance retained; valid duplicates deduplicate
Output-preservation matrix Five invalid CLI cases against a forced sentinel\n report Every command exits nonzero and preserves 9-byte SHA-256 b5f7e7d285029324d9b3acae19cc05099271454ac98bfc059a92b0581625cd51
Valid-output regression Existing validate integration fixture before versus after Existing raw and filtered accuracy expectations remain unchanged
Core: formatting/diff checks Direct rustfmt --edition 2021 --check on both changed Rust files; git diff --check upstream/master...HEAD; clean-worktree check Passed; only stable-rustfmt warnings for nightly-only repository settings were emitted; worktree clean

Tests not performed

  • The 1-GB direct-RNA slice was not run; the defect is isolated to truth-input validation and exact synthetic/parser oracles, and the complete workspace gate passed.
  • Performance, RSS, and I/O benchmarks were not run because this is a correctness/input-validation change.
  • cargo clippy was not run.

Scientific validation

  • Population/eligibility invariant: every accepted truth row reaches the truth map; no malformed row is silently removed.
  • Count/category conservation invariant: one (contig, strand, position) has at most one truth label; conflicts fail instead of changing category totals.
  • Coordinate/strand/interval invariant: BED intervals are nonnegative, nonempty, half-open; + and - are exact and independent.
  • Determinism invariant: conflicting inputs fail in both orders with symmetric provenance; identical overlaps are idempotent.
  • Independent oracle or specialist review: parser grammar, ChEBI precedence, coordinate semantics, overlap provenance, and complexity were independently reviewed from bioinformatics, Rust, and supervisor perspectives; no blocker remained.

Output and compatibility

  • User-visible change: invalid truth inputs now fail loudly and early; diagnostics are more specific.
  • Expected output differences: only inputs previously accepted through silent loss, invalid semantics, or conflict overwrite change.
  • Byte-identical controls: valid existing validate output remains unchanged.
  • CLI/API/schema compatibility: CLI and output schema unchanged; the guide clarifies the existing strand contract.
  • Partial-output or failure semantics: all truth parsing completes before report creation, so these input failures preserve an absent or pre-existing output.

Reviewer guide

  1. Review ground_truth_bed_reports_mixed_row_error_with_physical_line and ground_truth_bed_rejects_conflicting_overlap_in_both_orders for the primary parent-red contracts.
  2. Review parse_ground_truth_bed_line for coordinates, strand, and label grammar.
  3. Review parse_ground_truth_bed_file for explicit error propagation and interval-level conflict provenance.
  4. Review output-open ordering in ValidateFromModBam::run and the small documentation change.
  5. Rerun these canaries: cargo test -p mod_kit validate::subcommand::tests -- --test-threads=1 and cargo test -p modkit --test test_validate test_validate_bed_errors_preserve_output_and_report_line -- --exact.

Checklist

  • The issue contains reproducible observed and expected behavior.
  • The change is limited to the linked issue’s approved scope.
  • The regression is demonstrably red on the exact parent revision.
  • All tests actually performed are listed above with their results.
  • Unrun or inapplicable tests are disclosed.
  • Scientific counts/statistics and output compatibility are explicitly checked.
  • Formatting and diff-hygiene checks pass, or unrelated findings are documented.
  • No generated data, private sample identifiers, or unrelated changes are included.

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.

validate silently accepts invalid or conflicting truth BED rows

1 participant