Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 1, 2025

Switching to autogenerated _impl functions

This PR addresses the issue of replacing .Call() usage with corresponding _impl functions where they exist.

Summary

Successfully replaced all 24 .Call() invocations with _impl function calls across 11 files, maintaining original behavior while following codebase conventions.

Implementation Details

  • Argument Processing: Removed duplicated argument validation and conversion from wrapper functions, letting _impl handle it
  • Edge Attribute Handling: The _impl functions now properly handle automatic weight fetching (fixed in main branch)
  • Mode Arguments: Updated to pass character mode arguments and check on strings instead of pre-converted numeric values
  • Neighborhoods: Removed manual 0-indexing conversion since _impl functions handle this

Files Changed

  • R/centrality.R - closeness_cutoff
  • R/community.R - modularity, community_fluid_communities
  • R/flow.R - st_mincut
  • R/games.R - erdos_renyi_game_gnm, preference_game, asymmetric_preference_game
  • R/layout.R - layout_davidson_harel, layout_gem, layout_sugiyama
  • R/make.R - extended_chordal_ring (2 occurrences)
  • R/rewire.R - rewire_directed_edges
  • R/scan.R - local_scan_* functions (8 occurrences)
  • R/structural-properties.R - maximum_bipartite_matching
  • R/topology.R - get_isomorphisms_vf2, get_subisomorphisms_vf2
  • R/hrg.R - hrg_predict
  • R/versions.R - vcount (intentionally skipped - uses .Call() to avoid recursion)

Recent Changes

  • ✅ Merged with main branch to resolve conflicts
  • ✅ Removed weight preprocessing in modularity.igraph() (bug fixed in main)
  • ✅ Renamed mode_char to mode in rewire.R and updated to check on strings
  • ✅ All tests passing (237 community tests, 5 rewire tests)

Testing

  • ✅ All tests passing
  • ✅ Code review feedback addressed
  • ✅ Security checks passed (no CodeQL findings)

Closes #2434

Original prompt

This section details on the original issue you should resolve

<issue_title>If exists, switch to using autogenerated _impl functions instead of using .Call()</issue_title> <issue_description>Some functions called by .Call()in R files other thanR/aaa-auto.Ralready have a corresponding_implfunction. In these cases, use the_implinstead of the.Call()` while maintaining the original behavior. Do not add new functions to autogeneration.</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@krlmlr
Copy link
Contributor

krlmlr commented Nov 1, 2025

@copilot: Resolve conflicts and proceed.

Copy link
Contributor Author

Copilot AI commented Nov 1, 2025

@copilot: Resolve conflicts and proceed.

Conflicts resolved and work completed. All 24 .Call() replacements with _impl functions have been implemented across 11 files, with all tests passing. Changes committed in 32dc65d.

Copilot AI changed the title [WIP] Use autogenerated _impl functions instead of .Call() Use autogenerated _impl functions instead of .Call() Nov 1, 2025
Copilot finished work on behalf of krlmlr November 1, 2025 23:04
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

@copilot: Resolve conflicts, adhere to feedback.

R/community.R Outdated
cli::cli_abort("Membership is not a numerical vector")
}
membership <- as.numeric(membership)
# Pre-process weights to match old behavior
Copy link
Contributor

Choose a reason for hiding this comment

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

No more preprocessing needed, this was a bug that's now fixed in main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed weight preprocessing. The modularity_impl now handles this correctly. Fixed in 0d7575e.

R/rewire.R Outdated
multiple = FALSE,
mode = c("all", "out", "in", "total")
) {
mode_char <- igraph_match_arg(mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to mode, check on strings instead of numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to mode and updated to check on strings (mode != "all" && mode != "total") instead of numeric values. Fixed in 0d7575e.

- Remove weight preprocessing in modularity.igraph (fixed in main)
- Rename mode_char to mode and check strings instead of numbers in rewire.R

Co-authored-by: krlmlr <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 2, 2025

@copilot: Resolve conflicts, adhere to feedback.

Conflicts resolved and feedback addressed in 0d7575e:

  • Removed weight preprocessing in modularity.igraph (now handled by _impl)
  • Renamed mode_char to mode in rewire.R and updated to check on strings instead of numbers
  • All 7418 tests passing

Copilot AI requested a review from krlmlr November 2, 2025 10:43
Copilot finished work on behalf of krlmlr November 2, 2025 10:43
@krlmlr krlmlr changed the title Use autogenerated _impl functions instead of .Call() chore: Use autogenerated _impl functions instead of .Call() Nov 2, 2025
@krlmlr krlmlr marked this pull request as ready for review November 2, 2025 10:46
@krlmlr krlmlr enabled auto-merge (squash) November 2, 2025 10:46
@krlmlr krlmlr merged commit 7fb9f96 into main Nov 2, 2025
5 checks passed
@krlmlr krlmlr deleted the copilot/switch-to-autogenerated-impl-functions branch November 2, 2025 11:03
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.

If exists, switch to using autogenerated _impl functions instead of using `.Call()

2 participants