Skip to content

Conversation

@ning-y
Copy link

@ning-y ning-y commented Oct 26, 2025

Fixes #10153.

devtools::load_all(.)
library(SeuratData)
system.time(do.call(SCTransform, list(celegans.embryo)))

On main, this takes 25 minutes. On this branch, it takes 61 seconds.

In the interest of transparency this was generated by GitHub's Copilot w/ claude-sonnet-4.5.

Copilot AI review requested due to automatic review settings October 26, 2025 16:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a performance regression in the Parenting function that causes severe slowdowns (from 61 seconds to 25 minutes) when large objects are passed through do.call(). The fix replaces the original string-based call serialization approach with a more efficient extraction of function names from the call stack.

  • Replaces as.character(sys.calls()) with direct call parsing to avoid serializing large objects
  • Uses vapply for type-safe iteration over call stack entries
  • Handles both simple function names and qualified names (e.g., pkg::function)

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

Addresses PR comment feedback about incorrect handling of package-qualified
function names like pkg::func. Using paste(collapse='') would produce
'::pkgfunc' instead of the correct 'pkg::func'.

The deparse1() function properly reconstructs the syntax for all cases:
- Simple names: 'func'
- Qualified names: 'pkg::func'
- Operators: '[['

Verified:
- All existing tests pass
- Performance remains excellent (do.call 0.16x of direct call)
- Results are identical
Verified that direct call and do.call produce identical results:
- All data layers match byte-for-byte
- Variable features identical
- All tests pass

Explained apparent speed difference:
- First call pays ~7s warmup cost (code loading, compilation)
- Subsequent calls take ~0.78s for both methods
- Performance is truly identical after warmup

Results:
- Before fix: do.call took 1800+ seconds
- After fix: do.call takes 0.78 seconds (same as direct)
- Improvement: ~2300x faster
@ning-y ning-y marked this pull request as draft October 26, 2025 17:08
@ning-y ning-y marked this pull request as ready for review November 11, 2025 05:20
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.

BUG: SCTransform invoked with do.call fails to finish

1 participant