Skip to content

chore: Use autogen-impl functions for 3 motifs functions#2152

Merged
krlmlr merged 3 commits intomainfrom
motifs
Oct 19, 2025
Merged

chore: Use autogen-impl functions for 3 motifs functions#2152
krlmlr merged 3 commits intomainfrom
motifs

Conversation

@maelle
Copy link
Copy Markdown
Contributor

@maelle maelle commented Sep 22, 2025

Note that these high-level functions are tested.

@maelle maelle mentioned this pull request Sep 22, 2025
@maelle maelle changed the title Use autogen-impl functions for two motifs functions Use autogen-impl functions for 3 motifs functions Sep 22, 2025
Comment thread R/motifs.R
@@ -200,20 +200,14 @@ motifs <- function(graph, size = 3, cut.prob = NULL) {
count_motifs <- function(graph, size = 3, cut.prob = NULL) {
ensure_igraph(graph)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am on the fence about removing it, because even if that is asserted in the autogenerated function, shouldn't this happen before anything else?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you be more specific? You mean remove the ensure_igraph() call? Maybe for consistency with other functions keep it still?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@schochastics actually it's not very consistent among functions taking graph as input, from the little I've seen. A new upkeep task! 🤪

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am actually not surprised 🙈

@maelle maelle marked this pull request as ready for review September 22, 2025 08:26
@maelle maelle requested a review from schochastics September 30, 2025 08:35
@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Sep 30, 2025

Other comments @schochastics?

Copy link
Copy Markdown
Contributor

@schochastics schochastics left a comment

Choose a reason for hiding this comment

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

looks good to me :)

@maelle
Copy link
Copy Markdown
Contributor Author

maelle commented Oct 3, 2025

Thanks! However main is frozen (@krlmlr) so we can merge this after the release.

@maelle maelle mentioned this pull request Oct 17, 2025
95 tasks
@krlmlr krlmlr changed the title Use autogen-impl functions for 3 motifs functions chore: Use autogen-impl functions for 3 motifs functions Oct 19, 2025
@krlmlr krlmlr merged commit 2deff97 into main Oct 19, 2025
13 checks passed
@krlmlr krlmlr deleted the motifs branch October 19, 2025 02:45
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Oct 19, 2025

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants