feat: New voronoi_cells() to compute the Voronoi partitioning of a graph

[szhorvat]

@krlmlr @maelle Please have a look before I add a test.

[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 using Aviator.

---

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.

[maelle]

Thanks! I added a few comments.

[szhorvat]

Thanks for the review @maelle !

[krlmlr]

Looks good, thanks!

[krlmlr]

Why do the checks fail though?

[krlmlr]

Undocumented arguments in documentation object 'voronoi'
‘...’

See https://rlang.r-lib.org/reference/args_dots_empty.html?q=args_dots_e#null .

[szhorvat]

Thanks for the comments, @krlmlr. This PR is not urgent for me. I want to take this slow and use the opportunity to learn how to best do this.

[szhorvat]

This PR has revealed a larger bug: in auto-generated code, vectors created with R_SEXP_to_vector_int_copy are not freed. Can you please have a look at this @Antonov548? See the sanitizer failure. Similar memory leak bugs may be present in existing functions.

(But priority should be given to the "phoenix" milestone before this memory leak.)

[szhorvat]

@Antonov548 In typec-RC.yaml, I can see that some of non-allocating conversions were changed to allocating ones without adding cleanup code. Please always add the cleanup code as well, or if this is not trivial, do create a reminder issue to do it.

Whenever there is allocation in the INCONV section, there needs to be deallocation in the OUTCONV section. The IN part of at least VERTEX_INDICES, VERTEX_INDEX_PAIRS are affected. Can you add the deallocation and check if anything else is missing it?

It seems to me that there are lots of memory leaks at the moment.

[szhorvat]

@krlmlr Now that the memory leak issue is fixed, this PR is ready for merging or another review (but not urgent).

[krlmlr]

@maelle: Do these new featores conflict with the lifecycle work?

@szhorvat: Should we use a more descriptive name, perhaps voronoi_part() or voronoi_partitioning() ?

[szhorvat]

I think voronoi() is sufficiently descriptive, and consistent with how other functions are named. But it's good to be aware that there are several other R packages which have a function of the same name, which compute a geometric Voronoi tessellation: https://rdrr.io/search?q=voronoi Some of these are methods, sometimes it is even a class name. At this point it becomes a technical decision what to do, so I'll leave it to you.

If we do go with a different name, I'd like to use voronoi_cells().

Merging this is not urgent, it's fine to make this decision after the Tuesday release.

[maelle]

@maelle: Do these new features conflict with the lifecycle work?

@krlmlr no they do not, especially as adding an experimental function does not require registering for which version it was added.

[krlmlr]

Thanks. voronoi() -> voronoi_cells() sounds good to me, we'd also need to update the PR's title. Postponing.

[szhorvat]

Renamed to voronoi_cells().

[krlmlr]

Thanks!