Skip to content
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

Updated C igraph core to 0.8.1, updated tests and updated Travis build configuration #372

Closed
wants to merge 22 commits into from
Closed

Conversation

vtraag
Copy link
Member

@vtraag vtraag commented Feb 24, 2020

This PR is meant to update the R interface to the most recent igraph C core version.

There have been a number of API changes, which also need to be reflected in the R interface.

Some issues were fixed, which affected some of the tests (actually now leading to correct results, where some tests were previously incorrect). I've also updated all statements of the form expect_that(..., is_true()) to expect_true(...) (and similarly for the false variant), since I got too many warnings that is_true() was deprecated.

There are still a number of errors in the tests, which I need to check out. Incidentally, there also seems to be some bug in the avg_nearest_neighbour_degree when using a weighted graph, presumably there is an error there in the igraph C core, but I have not yet managed to nail it down and make a reproducible example. For now I commented out that test in order to proceed with other tests (otherwise it aborts with a free(): invalid pointer problem). There are also still some other errors in the tests that need to be resolved.

However, I already wanted to share the progress so far, to make sure that there are no obvious things I'm overlooking or doing incorrectly. Feedback is welcome @gaborcsardi.

Note that the functions.def was still incorrect in igraph. I have created PR igraph/igraph#1338 to address this, which of course affects to what commit the cigraph submodule should point.

The returned eigenvalues did not match the largest magnitude eigenvalues that are returned by the R eigen function.
@vtraag
Copy link
Member Author

vtraag commented Feb 26, 2020

I believe the PR is essentially good to go, and the tests pass locally. I only encounter one issue still: after all tests succeed, I receive an error:

Error in vapply(issues, issue_summary, FUN.VALUE = character(1)) : 
  values must be length 1,
 but FUN(X[[1]]) result is length 90

This seems to have to do with something with the reporter in the testthat framework, but I don't yet fully understand what exactly. I'm not sure what can be done about it, as it doesn't seem to be something that can be changed by any of the code in rigraph.

@gaborcsardi it seems that the PR build fails because r-builder does not support the latest R version (currently 3.6.2). I am not sure if it is still necessary to use the r-builder script? It seems that Travis supports building R packages without requiring specific r-builder scripts. Perhaps it would be possible to switch to building using the Travis supported configuration? I'd be happy to give it a try, but I don't have much experience with R.

vtraag added 9 commits March 11, 2020 12:32
It seems that because of r-lib/roxygen2#822 it
is necessary to first build the dll using compile_dll, otherwise the
NAMESPACE file will not be correctly generated.
First compiling the DLL will require a NAMESPACE file. Generating the
documentation and NAMESPACE using roxygen2 will require again *not* to
have a NAMESPACE file present. Hence, it first needs to be added, and
then needs to be removed again, in order to solve this catch-22.
@vtraag vtraag changed the title Updated to 0.8.0 and updated tests. Updated C igraph core to 0.8.1, updated tests and updated Travis build configuration Mar 13, 2020
@vtraag
Copy link
Member Author

vtraag commented Mar 13, 2020

@gaborcsardi , I updated the C core to version 0.8.1. Everything now builds correctly on R 3.5 and R 3.6. Two warnings remains:

  1. * checking pragmas in C/C++ headers and code ... WARNING This goes for GLPK included source files, I don't think we can correct this right now.
  2. Some problem with plfit:
Found ‘abort’, possibly from ‘abort’ (C)
    Object: ‘plfit/sampling.o’
  Found ‘rand’, possibly from ‘rand’ (C)
    Objects: ‘plfit/mt.o’, ‘plfit/sampling.o’

This will be corrected in the C core version 0.8.2, but this should not pose any problems.

The current development version of R shows a few issues, these mainly arise because class(obj) returns multiple values, most problematic cases involve c("matrix", "array"). Perhaps some of the class(obj) == "type" can be replaced by inherits(obj, "type")? I am not 100% whether this is the best approach. I assume that the current development version of R is R 4 which is due to appear on April 24, which still allows for some time to correct this, so this PR could still be merged.

If this PR can be merged, I can start working on adding some of the new functionality that has become available in the latest release of the C core. If there are any additional things that need to be done before this can be merged, let me know.

@szhorvat
Copy link
Member

@gaborcsardi Is it going to be an issue for this PR that recent R/igraph versions have not been pushed to this GitHub repo? The PR is therefore based on an older version than the current (the only version number I could see in this repo was 1.1 but the latest on CRAN is 1.2.5 ...)

@gaborcsardi
Copy link
Contributor

@szhorvat Latest releases are from the cran branch.

@vtraag
Copy link
Member Author

vtraag commented Mar 21, 2020

@gaborcsardi , I'm a bit lost as to the development on rigraph. In the contributing guide is says that contributions should be made against the dev branch. Do you (intend to) use the dev branch at all? Should this PR be (re)targeted at the cran branch? I'd be happy to rebase against the cran branch.

@gaborcsardi
Copy link
Contributor

I can merge the cran branch into dev. OTOH, the C lib repo also has a cran branch that is used in the R package...

@vtraag
Copy link
Member Author

vtraag commented Mar 21, 2020

OK, good, whatever is easiest to work with. Good to know that the cran branch from rigraph uses the cran branch from igraph, I wasn't aware. We should probably also merge back in those commits in the cran branch in the master branch, shouldn't we (cc @szhorvat and @ntamas) ?

@ntamas
Copy link
Member

ntamas commented Mar 21, 2020

We should probably also merge back in those commits in the cran branch in the master branch

Yes, it would be great.

@QuLogic
Copy link
Contributor

QuLogic commented Mar 22, 2020

It would be nice if, since this is based on a tagged version, this meant that rigraph could be compiled against an existing igraph library (#268).

@vtraag
Copy link
Member Author

vtraag commented Mar 22, 2020

@gaborcsardi , thanks for updating the dev branch, I will resolve the conflicts in the coming week.

@vtraag
Copy link
Member Author

vtraag commented Mar 23, 2020

We can close this in favour of PR #380.

@vtraag vtraag closed this Mar 23, 2020
@vtraag vtraag deleted the update/0.8.0 branch March 23, 2020 13:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants