Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 26, 2025

  • Merge main branch
  • Resolve merge conflicts
  • Remove spurious system headers from .dd files (cpp11.dd, rinterface.dd)
  • Move optional VERTEX subtraction logic outside of .Call() as requested by @maelle
  • Test changes - all tree tests pass (79 tests)
Original prompt

This section details on the original issue you should resolve

<issue_title>Standardize the handling of optional vertex parameters</issue_title>
<issue_description>The handling, and default value, of optional vertex parameters should be standardized, and Stimulus should be updated accordingly.

There are currently two C functions which take an optional vertex parameter: igraph_random_spanning_tree() and igraph_fundamental_cycles(). Only the former is exposed at the moment. We need a decision on this issue before I expose the latter.

In C, these two functions interpret a negative vertex index as not passing a vertex.

In R, the situation is more complicated because vertices can be referred to either by a string name or by an integer index. Both must be supported.

IMO there are two reasonable options:

  1. Signal that no vertex is provided by passing a non-positive value. The default parameter value should be 0. NULL is not allowed, nor are passing multiple vertices.
  2. Signal that no vertex is provided by passing NULL. The default parameter value should be NULL. Non-positive integers are not allowed. Multiple vertices are not allowed.

We currently have (1) mostly implemented. The auto-generated code looks like this:

  if (!is.null(vid)) vid <- as_igraph_vs(graph, vid)
  if (length(vid) == 0) {
    stop("No vertex was specified")
  }

However, currently, OPTIONAL VERTEX, without an explicit default, uses NULL as the default, which triggers the "No vertex was specified" error. If we go with choice (1), Stimulus should be updated to use 0 instead of NULL as the default. If we go with choice (2), the autogen code template must be updated for this.

@krlmlr, can you make a decision on this? When that is done, can you adapt Stimulus, @Antonov548 ?</issue_description>

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

@maelle On the R side, having `NULL` as default makes sense **for the interface**. Now, we can internally convert it to whatever the C side needs.

https://design.tidyverse.org/defaults-short-and-sweet.html</comment_new>
<comment_new>@krlmlr
I agree that the default on the interface should be NULL .

@Antonov548: Can you please help with Stimulus? Are the requirements clear?</comment_new>
<comment_new>@Antonov548

igraph_fundamental_cycles

Sorry, I'm not follow anymore. From the all pull requests and discussion I have thoughts that changes to Stimulus are not required.
Is there still anything I need to update in Stimulus?

Can you poine me please what should be updated?</comment_new>
<comment_new>@szhorvat
Since we're going with option 2, I think you're right. But to be sure, it'd be great to actually implement this (by removing the default of 0 from igraph_random_spanning_tree and letting it use the OPTIONAL default of NULL) and make sure it all works.</comment_new>


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

Copilot AI and others added 2 commits October 26, 2025 20:32
- Change default from `vid=0` to `vid=NULL` in YAML config
- Update Stimulus R generator to pass -1 (instead of 0) for optional VERTEX when NULL
- Update optional_wrapper_r to properly handle multi-line INCONV blocks
- Fix R auto-generated code to wrap length checks inside NULL checks
- Fix .Call to pass NULL correctly for optional vertex parameters
- Affects: random_spanning_tree, fundamental_cycles, vertex_path_from_edge_path

Co-authored-by: krlmlr <[email protected]>
Update sample_spanning_tree function in R/trees.R to use `vid = NULL` as default and update documentation accordingly.

Co-authored-by: krlmlr <[email protected]>
Copilot AI changed the title [WIP] Standardize handling of optional vertex parameters feat: standardize optional vertex parameters to use NULL default Oct 26, 2025
Copilot AI requested a review from krlmlr October 26, 2025 20:38
Copilot finished work on behalf of krlmlr October 26, 2025 20:38
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: Merge main, resolve conflicts as needed, and obey updated instructions.

R/aaa-auto.R Outdated
R_igraph_fundamental_cycles,
graph,
start - 1,
if (!is.null(start)) start - 1 else NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this logic should leave outside of the call. Above in if (!is.null(start)) { also already make start <- start - 1 and here in .Call just have start - 1 (I understand this means changing the Stimulus config file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a788d99. The subtraction now happens in the if (!is.null(start)) { block, and .Call() just passes start directly.

R/aaa-auto.R Outdated
R_igraph_random_spanning_tree,
graph,
vid - 1
if (!is.null(vid)) vid - 1 else NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a788d99 (same change as for fundamental_cycles_impl).

src/cpp11.dd Outdated
cpp11.o: \
cpp11.cpp \
igraph_types.hpp \
/home/runner/work/_temp/Library/cpp11/include/cpp11.hpp \
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems spurious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in edbb220. The system headers have been removed from cpp11.dd and rinterface.dd.

@krlmlr
Copy link
Contributor

krlmlr commented Nov 4, 2025

@copilot: Merge main, resolve conflicts, adhere to feedback.

Copilot AI and others added 2 commits November 4, 2025 21:58
Move the `- 1` subtraction for optional VERTEX parameters from .Call() into the INCONV block (inside the NULL check) as requested by @maelle. This improves code clarity by keeping all parameter transformations together.

Changes:
- Modified Stimulus R generator to add subtraction inside optional_wrapper_r for VERTEX type
- Updated CALL handling to pass VERTEX name directly for optional parameters
- Regenerated R/aaa-auto.R with cleaner code structure

Co-authored-by: krlmlr <[email protected]>
Copilot finished work on behalf of krlmlr November 4, 2025 22:06
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.

Standardize the handling of optional vertex parameters

3 participants