chore: Adapt handling of optional parameters to interface definition changes in the C core

[szhorvat]

This PR contains two changes:

• General cleanup in the auto-generated code
• Adapt to changed in this PR on the C side: refactor: use OPTIONAL instead of =NULL in interfaces igraph#2690

Both PRs should be merged at the same time.

There is some duplicate NULL-check ugliness that is not harmful, but should be cleaned up. I think this needs to be fixed directly in Stimulus. Can you help with this @Antonov548 ? Example: (Rf_isNull(weights) ? 0 : (Rf_isNull(weights) ? NULL : &c_weights))

I'm marking this as draft so it doesn't get accidentally merged before we all agree, but from my side it's good to go.

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.

---

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

[szhorvat]

The CI failures are due to the issue fixed in #1565 UPDATE: use #1568 instead. I'll rebase this once that PR is merged.

[szhorvat]

There is some duplicate NULL-check ugliness that is not harmful, but should be cleaned up. I think this needs to be fixed directly in Stimulus. Can you help with this @Antonov548 ? Example: (Rf_isNull(weights) ? 0 : (Rf_isNull(weights) ? NULL : &c_weights))

I was wrong, this needed to be fixed in the yaml interface file, and is now fixed.

I think this is good to go.

Since this is a big change, there is some risk and revdepchecks are in order. There are no functional changes, but there may be bugs.

[szhorvat]

#1569 shows that tests will pass after merging #1568.

[szhorvat]

The C side PR was merged, which means that this also needs to be merged. It is now updated to what is likely to become C/igraph 0.10.14.

I'd like to have this merged soon as it is a blocker for further improvements I want to make.

Thus, merge #1568, then merge this, then ping me.

[krlmlr]

CI/CD is failing here, https://github.com/igraph/rigraph/actions/runs/11681694081/job/32527681296?pr=1567#step:9:184 :

  Running examples in ‘igraph-Ex.R’ failed
  The error most likely occurred in:
  
  > base::assign(".ptime", proc.time(), pos = "CheckExEnv")
  > ### Name: count_motifs
  > ### Title: Graph motifs
  > ### Aliases: count_motifs
  > 
  > ### ** Examples
  > 
  > g <- sample_pa(100)
  > motifs(g, 3)
   [1]  NA  NA 218  NA  84   0   0   0   0   0   0   0   0   0   0   0
  > count_motifs(g, 3)
  [1] 302
  > sample_motifs(g, 3)
  Error in sample_motifs(g, 3) : 
    At vendor/cigraph/src/misc/motifs.c:604 : The number of vertices to use as a sample for motif count estimation must be at least one, got 0. Invalid value
  Execution halted

Haven't looked closer, but is this anticipated?

[szhorvat]

That's fixed in #1568, which replaces your #1565. Merge #1568 first and rebase.

[krlmlr]

I stacked and will review later today.

[szhorvat]

C core now updated to the very latest. This is basically at the 0.10.15 C release.

[krlmlr]

I'll split this PR. Do I understand correctly that the CRAN failure is fixed with #1568 and that this could wait until after the CRAN release?

[szhorvat]

I'll split this PR. Do I understand correctly that the CRAN failure is fixed with #1568 and that this could wait until after the CRAN release?

Somehow I missed this comment.

Yes, this is correct.

I still suggest including this in the release, since it's not possible to pick up the latest C core fixes without it. IMO splitting it up is not the best use of time when we are resource-constrained.

[krlmlr]

There now is a route towards merging this and the other open PRs.

For branches that start with each-, each commit is checked with CI/CD. This is important to me to support git bisect in case we need to later figure out if a regression happened: this really only works when each intermediate commit can be built and tested. While we could squash-merge this PR "as is", it's still too big for my taste.

I have started a branch with the next 20 commits: https://github.com/igraph/rigraph/compare/each-vendor . The proposed process is now:

• find the first commit where the checks fail
• port what's necessary (and only that) from this PR to the branch
• rebase-interactive to combine that change with the vendoring
• force-push, rinse and repeat

Once we're on par with the C core, I'll implement a process that vendors commits as they come, one by one.

CC @maelle @schochastics .

[krlmlr]

Making progress, already using two commits from this PR by necessity: main...refs/heads/each-vendor .

[krlmlr]

We're almost on par with the current C core, a little glitch with the plfit update: igraph/igraph@2cdc20a#r150944398 .

Do we plan a v0.10.16 release for the C core?

[szhorvat]

@krlmlr The rebase is done, and the target branch in GitHub is changed. I hope I didn't mess up anything.

[szhorvat]

Why are tests not running if the target branch is not main?

[krlmlr]

Good question. I only rarely use stacked PRs like this. We can retarget after applying the vendoring branch. Do the tests work for you locally?

[szhorvat]

Yes, devtools::test() pass.

I still have some trouble running the full check, including examples, in the same way as on CI, so I can't quite try that yet. What I mean is that I'm not sure how to run the full check even on main.

[krlmlr]

Thanks, this seems good enough for now.

[krlmlr]

The mainline is now in sync with igraph/C.

With the new history, are there now commits that you would group together?

Ideally, the tests should be representative of the health of the code -- no need to run examples etc. typically.

[krlmlr]

I can't find the UI to retarget the PR, this is now good to go to the main branch.

[szhorvat]

Hit this button to get the retarget UI:

[szhorvat]

I did the retargeting just now.

[szhorvat]

With the new history, are there now commits that you would group together?

I'm sorry about this being such a mess.

I'm thinking the whole thing might as well be squashed, as there's little value in keeping these commits separate. The changes I was making are not independent. E.g. removing some of these null checks is only possible if we use OPTIONAL instead of =NULL.

[krlmlr]

Thanks, great! This PR is really nice now.

My goal is to have the mainline bisect-able -- each intermediate commit should pass checks. In case we later see that this commit has created a regression, we can still split it. The PR would now be splittable, but much less so when it was intermingled with all the changes in the C core. There's no need to invest up front. The only reason to do so is to learn more about the changes now, I trust you there.

New changes in the C core should now also be imported automatically, commit by commit. I'll see to that. And we have a way to deal with a backlog of changes, just like that we had before today. We're in good shape!