-
-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: simple_cycles()
lists all simple cycles
#1580
base: simple-cycles-base-branch
Are you sure you want to change the base?
Conversation
Perhaps We can model this on |
245f000
to
3528646
Compare
Refactored to use It would be useful not to prevent running checks when the base branch is not |
3528646
to
e63943b
Compare
@krlmlr why is a review by @Antonov548 needed? After that review, I can work on the tests. |
The idea was just to take a look active pull requests to still keep up to date what is changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. The "simple cycles" feature is the first commit in the C core that breaks the R package. Almost ready to tackle it.
@@ -3580,25 +3580,6 @@ find_cycle_impl <- function(graph, mode=c("out", "in", "all", "total")) { | |||
res | |||
} | |||
|
|||
simple_cycles_impl <- function(graph, mode=c("out", "in", "all", "total"), min.cycle.length=-1, max.cycle.length=-1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you see a downside in keeping this and making simple_cycles()
a wrapper of this function?
This PR depends on #1567 and #1571. For now, it targets the
simple_cycles-base-branch
branch, which has those PRs merged. Once they are merged tomain
, it should be marked as ready-for-review and re-targeted.Can you please help me implement tests @maelle ? What I'd like to have a is a check on the cycle length histograms for the graph already added to the examples. Count cycles of length 1, length 2, etc. and add a check on the expected numbers. I'm not sure what's the best way to do this in R.
Closes #1573