feat: New `largest_component()` returns the largest connected component #786

ngmaclaren · 2023-04-19T20:54:19Z

Making this a draft pull request in case I've done something wrong.

krlmlr · 2023-04-19T21:00:03Z

Thanks. Can you please also commit the changes to the man/ directory (after devtools::document()), and push to the same branch?

ngmaclaren · 2023-04-19T21:06:10Z

So, locally go through the steps we discussed (R CMD INSTALL . ... devtools::document()), then push to the same branch? Do I need to resend the pull request?

ngmaclaren · 2023-04-19T21:09:15Z

You know, it helps to read the messages from GitHub. Let me get caught up locally again and push. I'll let you know when it's done. I'm concerned if I compile again that will cause problems for the merge (by creating a bunch of files?), but let's see what happens.

ngmaclaren · 2023-04-19T21:23:58Z

Ok, locally I opened a new R session in ./rigraph/ and ran:

pkgload::load_all()
testthat::test_local()
devtools::document()

Strangely, I can access the man page if I open R in the terminal, but not if I'm running R in Emacs/ESS. Regardless, I think it worked so I continued. There are now changes to NAMESPACE and man/largest_component.Rd. I pushed the commit to neil-lccfunc.

ngmaclaren · 2023-04-19T22:14:14Z

Hmm. That's a lot of angry red 'x' marks.

Rookie mistake! I think the problem is failing to include a variable definition in the man page example. Let me fix it and push again.

krlmlr · 2023-04-20T03:17:14Z

Thanks!

Checks are still failing, take a look at https://github.com/igraph/rigraph/actions/runs/4748452470/jobs/8437904643?pr=786#step:6:34 to see what's to be done (accessible by clicking "Details" in the "Checks" list above)
We need tests. Can you please run usethis::use_test("components") and add a few meaningful tests? What happens with an empty graph?
@maelle: There's ?components, and help topics for files in components.R . Do we need ?components ? Where do you think ?largest_component would point to?
Regarding the implementation: I wonder if we should just forward to decompose(max.comps = 1)[[1]] .

ntamas · 2023-04-20T06:27:11Z

I wonder if we should just forward to decompose(max.comps = 1)[[1]] .

I wouldn't; decompose() would have quite a lot of overhead if your graph consists of a giant component of size approx |V|/2 plus many many isolated nodes. You would create shedloads of single-vertex graphs only to throw them away a moment later.

szhorvat · 2023-04-20T07:54:14Z

What happens with an empty graph?

In the context of igraph we call the graph with zero vertices the null graph. When the argument is a null graph, this function should also return a null graph, for consistency with Python and Mathematica. See igraph/python-igraph#649

szhorvat · 2023-04-20T08:00:22Z

Regarding the implementation: I wonder if we should just forward to decompose(max.comps = 1)[[1]] .

decompose() does not return components in order of decreasing size, so this implementation is not correct.

R/components.R

ngmaclaren · 2023-04-20T10:28:48Z

@krlmlr I think the checks are failing because of the documentation organization issue:

-- Building function reference -------------------------------------------------
Error in `check_missing_topics()`:
! All topics must be included in reference index
✖ Missing topics: largest_component
ℹ Either add to _pkgdown.yml or use @keywords internal

I believe all I need to do to fix this is add at least one of #' @keywords, #' @rdname, or #' @family. But which one depends on where you want the function to go, I think. I can add

#' @rdname components
#' @family components

to match the roxygen2 documentation for component_distribution() just to see if it runs, but maybe I should hold off until @maelle responds?

ngmaclaren · 2023-04-20T11:05:07Z

What happens with an empty graph?

In the context of igraph we call the graph with zero vertices the null graph. When the argument is a null graph, this function should also return a null graph, for consistency with Python and Mathematica. See igraph/python-igraph#649

I agree with (2) in the linked issue. I didn't do it on purpose, but this function returns a null graph when the original is null:

> g <- make_empty_graph()
> g
IGRAPH c411815 D--- 0 0 -- 
+ > edges from c411815:
> largest_component(g)
IGRAPH 6a60570 D--- 0 0 -- 
+ > edges from 6a60570:
>

I guess that's because of consistency in the behavior of components() and induced_subgraph(): components(g)$membership is an empty vector for a null graph, V(g)[numeric(0)] returns 0/0 vertices, and induced_subgraph(V(g)[numeric(0)]) returns a null graph. Nice.

maelle · 2023-04-20T12:34:42Z

@krlmlr I think it should end up in https://r.igraph.org/reference/index.html#connected-components? Note the same help pages turn up in https://r.igraph.org/reference/index.html#structural-properties what do you @szhorvat @ntamas think?

maelle · 2023-04-20T12:37:16Z

@ngmaclaren congrats on your first PR! Sharing a (IMHO) useful tip: if you edit your very first comment (screenshot below) to include a line Fix #785, the PR will be linked to your issue, which will be indicated in GitHub interface and which means that merging this PR will close your issue. More info: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

ngmaclaren · 2023-04-20T12:44:33Z

@ngmaclaren congrats on your first PR! Sharing a (IMHO) useful tip: if you edit your very first comment (screenshot below) to include a line Fix #785, the PR will be linked to your issue, which will be indicated in GitHub interface and which means that merging this PR will close your issue.

Awesome---thank you! I made that change.

maelle · 2023-04-20T12:49:07Z

Yay! I can see it in the sidebar of this PR now:

ngmaclaren · 2023-04-20T13:31:39Z

Ok, I have simplified the function as recommended by @szhorvat and updated the documentation as recommended by @maelle . I wound up editing the documentation for components() in ./R/structural.properties.R to make it read better---I hope that's ok.

@krlmlr I have not added any tests yet. There is no test-components.R file in ./rigraph/tests/testthat/: do you want me to write some tests for the other functions in that family (e.g., components(), is_connected(), etc.)?

ngmaclaren · 2023-04-21T12:30:00Z

I'm afraid I'm not sure what to do with this error. The error is under Check windows-latest > Install R dependencies. Here's the relevant part of the message:

 Error: 
  ! error in pak subprocess
  Caused by error in `file(con, "rb")`:
  ! cannot open the connection
  ---
  Backtrace:
  1. pak::lockfile_install(".github/pkg.lock")
  2. pak:::remote(function(...) { …
  3. err$throw(res$error)
  ---
  Subprocess backtrace:
   1. base::withCallingHandlers(cli_message = function(msg) { …
   2. get("lockfile_install_internal", asNamespace("pak"))(...)
   3. plan$install()
   4. pkgdepends::install_package_plan(plan, lib = private$library, num_workers = nw, …
   5. base::withCallingHandlers({ …
   6. pkgdepends:::start_task(state, task)
   7. pkgdepends:::start_task_install(state, task)
   8. pkgdepends:::make_install_process(filename, lib = lib, metadata = metadata)
   9. pkgdepends:::detect_package_archive_type(filename)
  10. base::readBin(file, what = "raw", n = 6)
  11. base::file(con, "rb")
  12. base::.handleSimpleError(function (e) …
  13. global h(simpleError(msg, call))
  Execution halted
  Error: Process completed with exit code 1.

Is this something I can take care of on my end?

krlmlr · 2023-04-21T14:36:25Z

It's probably a bogus error. I triggered a new round of checks.

krlmlr · 2023-04-21T14:38:28Z

We have tests for other functions in that file in other test files. It would be great to have a corresponding test-*.R file for each *.R file, we're just not quite there yet. But for new developments we can certainly do this, and converge to the ideal situation step by step.

ngmaclaren · 2023-05-08T20:56:13Z

@krlmlr Do you all need anything else from me to close this out? Would you rather I include the tests in this PR? If so, do you want me to collect the tests for other components functions from other test-*.R files and put them into a test-components.R file?

We have tests for other functions in that file in other test files. It would be great to have a corresponding test-*.R file for each *.R file, we're just not quite there yet. But for new developments we can certainly do this, and converge to the ideal situation step by step.

krlmlr · 2023-05-20T16:15:26Z

Thanks for the heads-up. Tests in this PR would be great, if you like to align the tests that would be fine too, but not necessary. If you end up moving code, can you please put all such changes in (a) separate commit[s]?

krlmlr · 2023-05-20T16:16:03Z

I don't understand the Windows failures: https://github.com/igraph/rigraph/actions/runs/4765990318/jobs/8472547482 . They don't seem to occur on the main branch.

ngmaclaren · 2023-05-21T11:35:02Z

It looks like all checks have passed now? https://github.com/igraph/rigraph/actions/runs/5033072387

It looks like new checks ran yesterday.

ngmaclaren · 2023-05-21T11:46:32Z

Thanks for the heads-up. Tests in this PR would be great, if you like to align the tests that would be fine too, but not necessary. If you end up moving code, can you please put all such changes in (a) separate commit[s]?

Yes, no problem. I will work on this.

ngmaclaren · 2023-05-22T12:57:53Z

Updates:

I made a new file, test-components.R, and added my largest_component() tests to it.
Two functions in components.R didn't seem to have tests in testthat/, so I wrote some and added them to the same file.
These changes are marked as failing tests on ubuntu-20.04 oldrel 1 and 2, but I don't understand the error messages.

I haven't done any other consolidation.

krlmlr · 2023-05-22T13:27:42Z

Thanks! The test failures are from the main branch now.

ngmaclaren · 2023-05-22T13:29:10Z

Awesome! Thanks for your help!

Add function to return largest connected component

8b3e1a7

Updates to documentation

0fac725

Fix to man page example

220d70c

krlmlr changed the title ~~Add function to return largest connected component~~ feat: New largest_component() returns the largest connected component Apr 20, 2023

szhorvat reviewed Apr 20, 2023

View reviewed changes

R/components.R Outdated Show resolved Hide resolved

Improved documentation; simplified function

83b9b78

Merge branch 'main' into neil-lccfunc

e8cd536

ngmaclaren marked this pull request as ready for review April 26, 2023 18:22

Merge branch 'main' into neil-lccfunc

a758ba6

ngmaclaren added 4 commits May 22, 2023 07:52

Merge branch 'igraph:main' into neil-lccfunc

1f6fc6c

Create test-components.R

1409b64

Add tests for largest_component()

145b14c

Write tests for count_components() and component_distribution()

b3fa179

krlmlr merged commit a9fe0cd into igraph:main May 22, 2023

ngmaclaren deleted the neil-lccfunc branch May 22, 2023 13:29

github-actions bot locked as resolved and limited conversation to collaborators May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New `largest_component()` returns the largest connected component #786

feat: New `largest_component()` returns the largest connected component #786

ngmaclaren commented Apr 19, 2023 •

edited

Loading

krlmlr commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

krlmlr commented Apr 20, 2023

ntamas commented Apr 20, 2023

szhorvat commented Apr 20, 2023

szhorvat commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023 •

edited

Loading

maelle commented Apr 20, 2023

maelle commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

maelle commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

ngmaclaren commented Apr 21, 2023

krlmlr commented Apr 21, 2023

krlmlr commented Apr 21, 2023

ngmaclaren commented May 8, 2023

krlmlr commented May 20, 2023

krlmlr commented May 20, 2023

ngmaclaren commented May 21, 2023

ngmaclaren commented May 21, 2023

ngmaclaren commented May 22, 2023

krlmlr commented May 22, 2023

ngmaclaren commented May 22, 2023

feat: New largest_component() returns the largest connected component #786

feat: New largest_component() returns the largest connected component #786

Conversation

ngmaclaren commented Apr 19, 2023 • edited Loading

krlmlr commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

ngmaclaren commented Apr 19, 2023

krlmlr commented Apr 20, 2023

ntamas commented Apr 20, 2023

szhorvat commented Apr 20, 2023

szhorvat commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023 • edited Loading

maelle commented Apr 20, 2023

maelle commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

maelle commented Apr 20, 2023

ngmaclaren commented Apr 20, 2023

ngmaclaren commented Apr 21, 2023

krlmlr commented Apr 21, 2023

krlmlr commented Apr 21, 2023

ngmaclaren commented May 8, 2023

krlmlr commented May 20, 2023

krlmlr commented May 20, 2023

ngmaclaren commented May 21, 2023

ngmaclaren commented May 21, 2023

ngmaclaren commented May 22, 2023

krlmlr commented May 22, 2023

ngmaclaren commented May 22, 2023

feat: New `largest_component()` returns the largest connected component #786

feat: New `largest_component()` returns the largest connected component #786

ngmaclaren commented Apr 19, 2023 •

edited

Loading

ngmaclaren commented Apr 20, 2023 •

edited

Loading