chore: cascade gsource → gmulti (expression_matrix_class deprecation + overlap ids flip)#375
Merged
Conversation
setSpatialLocations, setDimReduction, setNearestNetwork, setSpatialNetwork, setPolygonInfo, and setSpatialEnrichment each had the same recursive empty-list prune inlined as a manually-nested if-cascade. .prune_nesting() already existed for setExpression — extending it to the other 6 setters eliminates ~60 lines of duplicate scaffolding. Also adds .vmsg_null_remove(verbose, what) to standardize the 'NULL passed to x. Removing specified <noun>.' message that appeared at each of those sites, with minor wording inconsistencies (some used wrap_msg + isTRUE(verbose), some used vmsg directly). All callsites now route through one helper. Net: 34 insertions / 90 deletions. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each setter had blocks like
if (isTRUE(verbose)) {
wrap_msg("...")
}
which collapse to a single vmsg(.v = verbose, "...") call — same
behavior, less scaffolding. Touched setters: setCellMetadata,
setFeatMetadata, setMultiomics, setSpatialLocations, setDimReduction,
setNearestNetwork, setSpatialNetwork, setSpatialGrid, setSpatialEnrichment.
Also includes:
- One wrap_txtf + isTRUE(verbose) site in setPolygonInfo's
polygon_name auto-pick block — converted to vmsg(... sprintf(...)).
- The NULL-removal blocks for setCellMetadata / setFeatMetadata /
setMultiomics / setSpatialGrid now route through the
.vmsg_null_remove helper added in the previous commit.
Bare wrap_msg calls in getters (getNearestNetwork, getSpatialEnrichment)
without a `verbose` param are intentionally left alone — they're
unconditional user-facing fallbacks.
No behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Now that every setter's verbose param is consumed only through vmsg() / .vmsg_null_remove() (both honor verbose = NULL via the package's giotto.verbose option resolution), align the defaults with setExpression / setPolygonInfo / setFeatureInfo / setGiottoImage which already use verbose = NULL. Touched setters: setCellMetadata, setFeatureMetadata, setMultiomics, setSpatialLocations, setDimReduction, setNearestNetwork, setSpatialNetwork, setSpatialGrid, setSpatialEnrichment. No behavior change for callers who pass verbose = TRUE/FALSE explicitly; default callers now get whatever giotto.verbose resolves to instead of unconditional TRUE. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
refactor: slot_accessors.R cleanup — .prune_nesting, vmsg, verbose=NULL
When a backed gobject's setNearestNetwork / setSpatialNetwork auto-write converts an in-memory igraph to a parquetEdgeStore, the @network slot on the nnNetObj / spatialNetworkObj becomes a dataStore. .evaluate_nearest_networks and .evaluate_spatial_network both walk into nnNetObj / spatialNetworkObj via the recursive nn_network[] <- .evaluate_nearest_networks(nn_network = nn_network[]) pattern. The recursive call previously had branches only for nnNetObj / igraph / data.frame. When @network was already a parquetEdgeStore, none matched; the function fell through and returned implicit NULL — which the recursive assignment then wrote back, silently nullifying @network. Downstream code then hit "Must provide a graph object" from igraph called on NULL. Add an `inherits(x, "dataStore")` early-return branch in both validators. dataStore-backed networks are validated at write time by GiottoDisk (sourceWrite checks the input igraph / data.table; the resulting parquetEdgeStore carries the validated edge schema), so re-validating in the giotto context is a pass-through. NOTE — partial fix: this prevents @network nullification but doesn't unblock the full external-adoption path that exercises .ss_gdsrc_register_external_network in GiottoDisk. The remaining gap is in spatIDs(nnNetObj) / spatIDs(spatialNetworkObj) which call igraph::V(x@network) and x[]$from directly without dataStore branches. Fixing those cleanly likely wants a `nodeIDs()` generic that GiottoDisk can specialize for parquetEdgeStore — separate piece of work tracked in memory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-accepts-dataStore fix: .evaluate_*_network accept dataStore-backed @network (partial)
…taStore When the network setter auto-write path or sourceAdopt puts a parquetEdgeStore (or any dataStore subclass) into @network, the existing spatIDs methods called igraph::V(x@network) and x[]\$from directly — failing with "Must provide a graph object" because @network was no longer an igraph. Add a leading branch in both methods: if @network inherits dataStore, delegate to spatIDs(@network) so dispatch picks up the type-specific implementation (e.g. GiottoDisk's spatIDs(parquetEdgeStore)). Fall back to the canonical igraph path otherwise. Together with PR giotto-suite#363 (validators accept dataStore) and GiottoDisk's spatIDs(parquetEdgeStore) (just landed), this completes the end-to-end path for parquetEdgeStore-backed networks through giotto initialize, .check_nearest_networks, etc. Two new tests in test-networks.R cover the delegation: - spatIDs(nnNetObj) returns node IDs when @network is parquetEdgeStore - spatIDs(spatialNetworkObj) same Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dataStore fix: spatIDs(nnNetObj/spatialNetworkObj) delegate when @network is dataStore
…elpers The expression_matrix_class param was already deprecated in createExprObj but still had real defaults on createGiottoObject, readExprData, and readExprMatrix. createGiottoObject forwarded the default into readExprData → .read_expression_data → createExprObj, which then matched is_present() and emitted the deprecation warning on every default createGiottoObject call — generating thousands of warnings across the test suite. Three changes, same pattern as createExprObj: - Default changed from a real character vector to lifecycle::deprecated() - is_present() check emits the deprecation warning when the param is actually supplied by the caller - Param is no longer forwarded to downstream helpers .read_expression_data (internal) drops the param entirely — no external callers, no longer plumbed through. The DelayedArray / dbSparseMatrix branches in readExprMatrix are removed since the param can no longer influence return type; the function now always returns the default sparse class. Acceptable behavior change for a deprecated parameter — surfaced via the warning. Test suite: 967 / 0 (was previously generating ~3000 deprecation warnings on every run from the cascade; warning count down to 1449). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ression-matrix-class deprecate: expression_matrix_class across createGiottoObject + read helpers
- pulls `expression_matrix_class ` deprecation and a docs change
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>
flip overlapPointDT[i] / [,j] default: ids = TRUE → FALSE
Cascade giotto-suite#374 (flip overlapPointDT ids default TRUE → FALSE) up to gsource.
Cascade giotto-suite#372 (expression_matrix_class deprecation) + giotto-suite#374 (flip overlapPointDT[i] / [,j] default ids = TRUE → FALSE) into gmulti.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cascade the recent `gsource` changes up to `gmulti`:
Test plan