-
-
Notifications
You must be signed in to change notification settings - Fork 200
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: Upgrade igraph/C to 0.10 #840
Conversation
@krlmlr Related to this I have question: How to deal with input and output vectors which in C igraph are 64 integer and on R should be double? Does it mean that we need to always copy such vectors with type conversion? Since currently we just have use memory mapping for input vectors and for output vectors just memory copy. And should we keep logic of memory mapping for C double vectors? |
I don't follow. AFAICT we no longer are using memory mapping in the main branch. |
Oh, we do, in |
It makes sense to avoid a copy when handling vectors of doubles, otherwise performance may be severely impared in some cases. Consider weighted graphs: there's no reason to copy the weigths if they are already stored as doubles. Some algorithms will only access a few of the edges, so avoiding copying all edge weights will be a significant saving. |
Oh, right! So, we need to be careful about which instances to change. |
Okay. I will start update of types conversion in new pull request. |
What are the next steps here? |
I think to have such next steps:
|
Thanks. This still has weird compilation errors mentioning |
I have errors related to linking in R. On gitpod I also haven't problem with |
Can't build locally, with weird errors. Trying in https://github.com/cynkra/rig-ubuntu now. |
Can it be because different |
In the Docker image:
|
Priorities:
We're on a good path! Thanks for your efforts. |
I have compiling rigraph with 0.10 (Still some warnings which should be fixed). Now it's time to fix R code and communication R with C. I have some weird errors when I try to run tests. @krlmlr Would be great if you can check it. |
What do the errors look like? Does it happen in the Docker image? |
int R_SEXP_to_igraph(SEXP graph, igraph_t *res) {
res->n=R_igraph_get_n(graph);
res->directed=R_igraph_get_directed(graph);
R_igraph_get_from(graph, &res->from);
R_igraph_get_to(graph, &res->to);
R_igraph_get_oi(graph, &res->oi);
R_igraph_get_ii(graph, &res->ii);
R_igraph_get_os(graph, &res->os);
R_igraph_get_is(graph, &res->is);
/* attributes */
REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[0] = 1; /* R objects refcount */
REAL(VECTOR_ELT(VECTOR_ELT(graph, igraph_t_idx_attr), 0))[1] = 0; /* igraph_t objects */
res->attr=VECTOR_ELT(graph, igraph_t_idx_attr);
return 0;
} Why are the fields copied one by one instead of just copying the entire Note that in 0.10 there's also a cache entry which must be set correctly. Is there an problem I'm missing with a simple |
Similarly, I am not really familiar with the R attribute handler. You'll need to make sure that the |
It was just changed iterativly. Firstly with getters of fields, then with pointer to |
You need to handle the cache somehow before 0.10 will work correctly. In fact, is it even necessary to copy one The problem with making refactoring too incremental is that you won't see the forest for the tree. Some changes can't be decoupled from each other. |
Oh, I see. For caching it's indeed required to change. |
|
0.9 does not use a cache, so there's no issues with setting or copying the cache. 1.5.0 is most likely working fine, just not the prettiest. |
I don't mind fixing and releasing 1.5.1 so that the 0.10 version works correctly from the start. |
I see at lease first tests from test suite is failing due to the cache in graph. I think we need to add cache support to rigraph. Not sure how we should manage cache on R side. But I will start look in this direction. |
The cache does not need to be managed explicitly. Just use public API functions to modify Is there any instance when this cannot be done, other than setting the attributes? Also, is it possible to avoid using |
@ntamas @szhorvat @GroteGnoom: I could use more eyes for the changes to |
I see some instances of |
There are multiple instances of function calls without checking for errors. |
There is no clarity about which functions return an error code an which don't. There are still a lot of Something I mentioned before, and I believe is an absolute must before any 2.0 release, is to have additional clarity about which function is "top level", i.e. is called directly from R, and which function is called by other C function. Errors must be handled differently in these two, with |
Lots of deprecated functions are still used. These should be brought up to date. Either this is very easy (so do it now) or it might require some non-trivial preparation (which means we need to become aware of it now). |
That's all from me for now for |
Thanks. The 1.6.0 release is still being processed. Happy to merge ASAP after that. |
Great! Do you mind if I make some fixes here until that happens, such as replacing deprecated functions? |
Thanks. It makes no difference if we fix here or after merging t the main branch, as long as the builds in this branch remain clean. |
Good. Looking forward to this being merged into |
igraph 1.6.0 is on CRAN. Entering anarchy. All unresolved discussion points in this PR should become issues in https://github.com/igraph/rigraph/milestone/14. |
Congrats @Antonov548 @szhorvat @ntamas, thanks for the great work! |
based on: #768
@param normalized TBD
get.incidence.dense()
#' if
skip("Waiting for #935")
as_adjacency_matrix
🔴is_tree
auto-generation (auto-generated code is currently manually modified) 🔴ggnetwork
, see feat: Upgrade igraph/C to 0.10 #840 (comment) (@krlmlr opened aggnetwork
PR)rgraph6
, fixed in package (@szhorvat)molnet
, PR submitted (@szhorvat)oddnet
, PR submitted (@szhorvat)malan
, issue opened (@szhorvat) Upcoming change to igraph's girth() function affects this package mikldk/malan#18DiagrammeR
, issue opened (@szhorvat) Test relies on specific output from sample_islands() for given seed rich-iannone/DiagrammeR#500migraph
, this is due to a breaking change inigraph_power_law_fit
in 0.10, which should be mirrored on the R side. See igraph 0.10 changelog, and ask @ntamas for advice if needed.'names' attribute [100] must be the same length as the vector [0]
dnet
get.all.shortest.paths
should returnres
wpa
igraph_community_fluid_communities
return onlymembership
netcom
, incorrect use oftransitivity(..., type='weighted')
, notify package. See feat: Upgrade igraph/C to 0.10 #840 (comment)Canek
assumes a fixed output fromcluster_louvain()
for the same seed across igraph versions, which we don't guarantee. But I still don't quite see why this changed. Further investigation may be useful. See feat: Upgrade igraph/C to 0.10 #840 (comment)dnet
,rags2ridges
andnetrankr
failures due to the$res
component of theall_shortest_paths()
result not being present (@krlmlr)double
, then converted toigraph_integer_t
. 🟡int
, then converted to the appropriate enum type.double
->igraph_integer_t
conversion.REAL(vec)[0]
) — waiting on refactor: make_empty_graph() is now fully auto-generated #1068int
can hold, but the number of matrix elements is not. 🟡igraph_invalidate_cache()
and use it as appropriate in tests 🟡Closes #753