igraph / rigraph

igraph R package
https://r.igraph.org
555 stars 202 forks source link

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

Open szhorvat opened 4 weeks ago

szhorvat commented 4 weeks ago

This PR contains two changes:

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[bot] commented 4 weeks ago

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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

1569 shows that tests will pass after merging #1568.

szhorvat commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

krlmlr commented 3 weeks ago

I stacked and will review later today.

szhorvat commented 3 weeks ago

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

szhorvat commented 1 week ago

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.