Skip to content

flip overlapPointDT[i] / [,j] default: ids = TRUE → FALSE#374

Merged
jiajic merged 1 commit into
giotto-suite:devfrom
jiajic:flip-overlap-ids-default
May 26, 2026
Merged

flip overlapPointDT[i] / [,j] default: ids = TRUE → FALSE#374
jiajic merged 1 commit into
giotto-suite:devfrom
jiajic:flip-overlap-ids-default

Conversation

@jiajic

@jiajic jiajic commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Flip the default for `[` on `overlapPointDT` single-axis indexing from `ids = TRUE` (return integer indices) to `ids = FALSE` (return subset object). The selection-query form is still available via `ovlp[i, ids = TRUE]`.

Why

The previous default was the unusual case. Most callers want subset semantics:

  • `.subset_overlaps_poly` used `[i, ids = FALSE]` to work around the default
  • `.subset_overlaps_feat` used `[, i, ids = FALSE]` for the same reason
  • `overlapIntensityDT[i]` has no `ids` param — always returns subset
  • `SpatVector[i]` (legacy storage) also returns subset
  • The "return indices" default was specific to `overlapPointDT`, creating an asymmetry across the three classes that store overlap data

Audit found zero external callers (Giotto, GiottoDisk, GiottoVisuals) relying on the old default — only the test in `test-aggregate.R` and the documented examples.

What changes

  • Two `setMethod("[", "overlapPointDT", ...)` defaults flipped + branch order inverted for readability (explicit `ids = TRUE` selects, default subsets).
  • `.subset_overlaps_poly` / `.subset_overlaps_feat` simplified to `[i]` / `[, i]` — the `ids = FALSE` workaround is no longer needed.
  • `test-aggregate.R`: the selection-query tests now pass `ids = TRUE` explicitly; new assertions cover the default subset behavior.
  • Class docs updated to reflect the new default.

Side benefit

Standalone polygon loads (e.g. `GiottoData::loadSubObjectMini`) that carry legacy `SpatVector` overlaps now subset cleanly. `SpatVector[i]` returns a subset `SpatVector`, which is what `.subset_overlaps_poly` now expects. No migration hook required.

Test plan

  • Full test suite: 970 / 0.
  • Standalone-loaded polygon with legacy SpatVector overlaps subsets correctly (`GiottoData::loadSubObjectMini("giottoPolygon")` + `gpoly[ids]` + `gpoly[c(T,F)]`).
  • Both forms still work via tests: `ovlp[i]` (default, subset) and `ovlp[i, ids = TRUE]` (selection, integer vector).

🤖 Generated with Claude Code

Default subset on a single axis now returns an overlapPointDT subset
object, matching overlapIntensityDT (which has no ids param and always
subsets) and SpatVector (legacy storage). The selection-query form
(integer indices) is still available via `ovlp[i, ids = TRUE]`.

Why: callers consistently want subset semantics — `.subset_overlaps_poly`,
`.subset_overlaps_feat`, and the broken-then-reverted-then-broken
"ids = FALSE" workarounds all converge on subset. The default
returning indices was the unusual case (only documented + tested, no
external callers found). Flipping it removes a footgun.

Internal callers updated:
- `.subset_overlaps_poly`: `[i, ids = FALSE]` → `[i]`
- `.subset_overlaps_feat`: `[, i, ids = FALSE]` → `[, i]`

Selection-query callers explicit:
- test-aggregate.R: now uses `ovlp[i, ids = TRUE]` for the indices form,
  plus new test asserting `ovlp[i]` default returns overlapPointDT.

Docs in classes-overlaps.R updated to reflect the new default.

Side benefit: standalone polygon loads (e.g. GiottoData::loadSubObjectMini)
that carry legacy SpatVector @overlaps now subset cleanly — SpatVector's
[i] already returns a subset SpatVector, so .subset_overlaps_poly works
without needing the ids workaround that SpatVector's [ doesn't support.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jiajic jiajic merged commit ee94bfa into giotto-suite:dev May 26, 2026
2 checks passed
jiajic added a commit to jiajic/GiottoClass that referenced this pull request May 26, 2026
Cascade giotto-suite#374 (flip overlapPointDT ids default TRUE → FALSE) up to gsource.
jiajic added a commit to jiajic/GiottoClass that referenced this pull request May 26, 2026
Cascade giotto-suite#372 (expression_matrix_class deprecation) + giotto-suite#374 (flip
overlapPointDT[i] / [,j] default ids = TRUE → FALSE) into gmulti.
jiajic added a commit that referenced this pull request Jun 1, 2026
overlapPointDT-class.Rd reflects PR #374 (ids = TRUE → FALSE default,
updated examples). spatRelate.Rd reflects PR #376 (trimmed
cross-package GiottoDisk context). Source roxygen was updated in those
PRs but the generated .Rd files were not refreshed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jiajic added a commit that referenced this pull request Jun 1, 2026
Brings in doc regen for overlapPointDT (PR #374 ids flip) and spatRelate
(PR #376 cross-package trim) from upstream/dev #377.
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.

1 participant