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

szhorvat · 2024-10-31T19:09:49Z

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 · 2024-10-31T19:09:54Z

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 · 2024-10-31T20:38:47Z

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 · 2024-10-31T22:39:06Z

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 · 2024-11-03T12:27:05Z

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

szhorvat · 2024-11-05T09:57:38Z

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 · 2024-11-07T09:20:04Z

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 · 2024-11-07T10:02:16Z

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

krlmlr · 2024-11-07T10:27:03Z

I stacked and will review later today.

szhorvat · 2024-11-07T13:52:12Z

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?

R/aaa-auto.R

tools/stimulus/functions-R.yaml

…rated code and remove support for deprecated array3 type

chore: fix some spelling mistakes refactor: minor readability cleanup in igraph_decompose() doc: more documentation improvements doc: several documentation improvements and cross-referencing chore: fix typo in error message docs: community_optimal_modulairty() does support directed graphs chore: do not warn about unknown warning options with legacy Intel compiler fix: validate sample size in igraph_motifs_randesu_estimate() chore: fix typo in comment

…d missing IGRAPH_R_CHECK error checks

fix: some BIPARTITE_TYPES parameters were incorrectly marked as optional refactor: use OPTIONAL instead of =NULL in interfaces refactor: minor readability cleanup in matching functions fix: `igraph_bipartite_projection_size()` now validates the bipartite `types` vector

…' in igraph_is_bipartite

fix: some weights parameters were incorrectly marked as OPTIONAL in functions.yaml fix: mark some optional weight parameters as OPTIONAL in functions.yaml fix: type -> types in is_bipartite interface

…n_size()` This parameter is optional in R, but not in C. Therefore the C interface definition doesn't provide a default or OPTIONAL marker.

interface: add more missing OPTIONAL markers

These checks are added automatically by Stimulus for OPTIONAL parameters

interface: even more missing OPTIONAL markers

chore: trim trailing whitespace in interface file interface: add missing interface for igraph_get_isomorphisms_vf2_callback() interface: mark all callback extra parameters and some callback functions as optional interface: add default parameter values for fundamental_cycles() and minimum_cycle_basis() interface: even more missing OPTIONAL markers interface: add more missing OPTIONAL markers fix: some weights parameters were incorrectly marked as OPTIONAL in functions.yaml fix: mark some optional weight parameters as OPTIONAL in functions.yaml fix: type -> types in is_bipartite interface fix: some BIPARTITE_TYPES parameters were incorrectly marked as optional refactor: use OPTIONAL instead of =NULL in interfaces feat: functionality for listing all simple cycles interface: default OUT mode for igraph_find_cycle() refactor: better error reporting for igraph_find_cycle() chore: some copyright header cleanup in public headers refactor: minor readability improvements fix: add some missing IGRAPH_CHECKs

…ycles_callback()` as the framework for handling callback functions is not yet present

chore: updated changelog chore: updated changelog chore: run pre-commit hooks chore: bump VERSION property in shared library fix: fix self-loops handling during simple cycle detection, closes igraph/igraph#2692 Revert "fix: temporary fix for simple cycle search mishandling self-loops, ref igraph/igraph#2692" fix: temporary fix for simple cycle search mishandling self-loops, ref igraph/igraph#2692 test: minimum cycle length for igraph_simple_cycles() refactor: do not use vector_get() in simple cycle finder refactor: cleaner parameter ordering in igraph_i_simple_cycles_circuit() refactor: support specifying a minimum cycle length in `igraph_simple_cycles()` interface: loops=True default for igraph_degree() chore: updated contributors list

… widest path functions This parameter is not optional in C, but it still needs a default in R to signify that weights should be taken from attributes.

szhorvat · 2024-11-20T09:58:55Z

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.

szhorvat requested review from ntamas, krlmlr and Antonov548 October 31, 2024 19:09

This was referenced Oct 31, 2024

refactor: use OPTIONAL instead of =NULL in interfaces igraph/igraph#2690

Merged

Bad conversion of NULL when passing integer vector #1564

Open

szhorvat marked this pull request as ready for review October 31, 2024 22:37

szhorvat changed the title ~~Adapt handling of optional paramters to interface definition changes in the C core~~ Adapt handling of optional parameters to interface definition changes in the C core Nov 4, 2024

szhorvat mentioned this pull request Nov 5, 2024

Expose simple_cycles() #1573

Open

krlmlr changed the title ~~Adapt handling of optional parameters to interface definition changes in the C core~~ chore: Adapt handling of optional parameters to interface definition changes in the C core Nov 7, 2024

krlmlr force-pushed the refactor/clean-up-OPTIONAL branch from 5d7abb4 to 34bcf0c Compare November 7, 2024 10:26

krlmlr force-pushed the refactor/clean-up-OPTIONAL branch from 34bcf0c to 2bce24f Compare November 7, 2024 13:24

krlmlr reviewed Nov 7, 2024

View reviewed changes

R/aaa-auto.R Outdated Show resolved Hide resolved

tools/stimulus/functions-R.yaml Show resolved Hide resolved

krlmlr mentioned this pull request Nov 7, 2024

chore: Add checks to generated code #1577

Open

krlmlr force-pushed the refactor/clean-up-OPTIONAL branch from a73a1ca to 26f0dfa Compare November 8, 2024 07:19

szhorvat mentioned this pull request Nov 11, 2024

feat: simple_cycles() lists all simple cycles #1580

Draft

szhorvat added 6 commits November 18, 2024 09:19

refactor: use OPTIONAL instead of =NULL consistently in functions-R.yaml

5d292e3

refactor: use IGRAPH_R_CHECK at top-level error checking in auto-gene…

f021a84

…rated code and remove support for deprecated array3 type

fix: use NULL instead of 0 where appropriate in interface code and ad…

ec1febc

…d missing IGRAPH_R_CHECK error checks

refactor: prepare for C core interface standardizing 'type' -> 'types…

8cdc23e

…' in igraph_is_bipartite

szhorvat added 10 commits November 18, 2024 09:19

chore: Update vendored sources to igraph/igraph@7eb3949

2e686ff

fix: some weights parameters were incorrectly marked as OPTIONAL in functions.yaml fix: mark some optional weight parameters as OPTIONAL in functions.yaml fix: type -> types in is_bipartite interface

hack: provide NULL default for types argument of `bipartite_projectio…

dc83b1a

…n_size()` This parameter is optional in R, but not in C. Therefore the C interface definition doesn't provide a default or OPTIONAL marker.

chore: Update vendored sources to igraph/igraph@f852b3d

ba6615d

interface: add more missing OPTIONAL markers

fix: remove some unneeded null checks in the auto-generated code

2577a2f

These checks are added automatically by Stimulus for OPTIONAL parameters

chore: Update vendored sources to igraph/igraph@a557e9f

e11ea97

interface: even more missing OPTIONAL markers

fix: add another missing error check in auto-generated code

1da71da

fix: temporarily disable generating an interface for `igraph_simple_c…

18dad62

…ycles_callback()` as the framework for handling callback functions is not yet present

interface: re-add NULL default value for the weights parameter of…

305bc33

… widest path functions This parameter is not optional in C, but it still needs a default in R to signify that weights should be taken from attributes.

szhorvat force-pushed the refactor/clean-up-OPTIONAL branch from c1d0791 to 305bc33 Compare November 18, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

szhorvat commented Oct 31, 2024

aviator-app bot commented Oct 31, 2024

szhorvat commented Oct 31, 2024 •

edited

Loading

szhorvat commented Oct 31, 2024 •

edited

Loading

szhorvat commented Nov 3, 2024

szhorvat commented Nov 5, 2024

krlmlr commented Nov 7, 2024

szhorvat commented Nov 7, 2024 •

edited

Loading

krlmlr commented Nov 7, 2024

szhorvat commented Nov 7, 2024

krlmlr left a comment

szhorvat commented Nov 20, 2024 •

edited

Loading

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

Are you sure you want to change the base?

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

Conversation

szhorvat commented Oct 31, 2024

aviator-app bot commented Oct 31, 2024

Current Aviator status

How to merge

szhorvat commented Oct 31, 2024 • edited Loading

szhorvat commented Oct 31, 2024 • edited Loading

szhorvat commented Nov 3, 2024

szhorvat commented Nov 5, 2024

krlmlr commented Nov 7, 2024

szhorvat commented Nov 7, 2024 • edited Loading

krlmlr commented Nov 7, 2024

szhorvat commented Nov 7, 2024

krlmlr left a comment

Choose a reason for hiding this comment

szhorvat commented Nov 20, 2024 • edited Loading

szhorvat commented Oct 31, 2024 •

edited

Loading

szhorvat commented Oct 31, 2024 •

edited

Loading

szhorvat commented Nov 7, 2024 •

edited

Loading

szhorvat commented Nov 20, 2024 •

edited

Loading