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

chore: Update vendored sources to igraph/igraph@03760a09cc7195dfaad3a1e60cff1632aa6a6118 #1470

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

szhorvat
Copy link
Member

@szhorvat szhorvat commented Aug 22, 2024

@krlmlr For revdepcheck purposes, as agreed. Somehow I don't see any deprecation warnings, despite igraph_minimum_spanning_tree_* functions being deprecated. @Antonov548, are deprecation warning disabled now? I don't see where.

Copy link
Contributor

aviator-app bot commented Aug 22, 2024

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

@szhorvat
Copy link
Member Author

szhorvat commented Aug 22, 2024

Why are checks not running? Do I need to mark as ready-for-review? EDIT: No, checks are still not running.

@szhorvat szhorvat marked this pull request as ready for review August 22, 2024 09:19
R/aaa-auto.R Outdated
@@ -1370,6 +1370,27 @@ feedback_arc_set_impl <- function(graph, weights=NULL, algo=c("approx_eades", "e
res
}

feedback_vertex_set_impl <- function(graph, weights=NULL, algo=EXACT_IP) {
Copy link
Member Author

@szhorvat szhorvat Aug 22, 2024

Choose a reason for hiding this comment

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

@Antonov548 Again, algo=EXACT_IP is invalid code, which needs to be dealt with, either by setting up translation for the new type, or removing code generation for this function temporarily. Exposing this function should be really easy, as it's near-identical to the feedback arc set one (so it's a copy and paste job). Ref: #1447 I recommend exposing it right away.

@szhorvat
Copy link
Member Author

Disabled codegen for all distances functions, since these are exposed manually, and moving them to autogen is unlikely to happen in the near future due to the custom interface. The extra commit finally allows checks to run.

@Antonov548
Copy link
Contributor

@Antonov548, are deprecation warning disabled now? I don't see where.

@szhorvat deprecation warnings are disabled in igraph_config.h in order to pass CI and CRAN. You can define IGRAPH_DEPRECATED_ENUMVAL to see deprecation warnings.

@szhorvat
Copy link
Member Author

@szhorvat deprecation warnings are disabled in igraph_config.h in order to pass CI and CRAN. You can define IGRAPH_DEPRECATED_ENUMVAL to see deprecation warnings.

I think this is perfect for now.

@szhorvat szhorvat force-pushed the try/c-core-2024-08-22 branch from 259abce to fa9dca7 Compare August 22, 2024 17:30
@szhorvat szhorvat changed the title chore: Update vendored sources to igraph/igraph@d37be6e5a28bc6c299b4656313b7255c8378c17c chore: Update vendored sources to igraph/igraph@03760a09cc7195dfaad3a1e60cff1632aa6a6118 Aug 22, 2024
@krlmlr krlmlr force-pushed the try/c-core-2024-08-22 branch from fa9dca7 to f68f630 Compare August 22, 2024 20:03
@krlmlr krlmlr closed this Aug 22, 2024
@krlmlr krlmlr reopened this Aug 22, 2024
@krlmlr krlmlr force-pushed the try/c-core-2024-08-22 branch from f68f630 to 9733b8d Compare August 22, 2024 20:04
@krlmlr
Copy link
Contributor

krlmlr commented Aug 22, 2024

Checks run in #1472. I have no idea what's going on.

@krlmlr krlmlr closed this Aug 22, 2024
@krlmlr
Copy link
Contributor

krlmlr commented Aug 22, 2024

No, they are not!

@krlmlr krlmlr reopened this Aug 22, 2024
@szhorvat szhorvat force-pushed the try/c-core-2024-08-22 branch from c264b1e to c282139 Compare August 24, 2024 20:17
@szhorvat
Copy link
Member Author

Tests pass, if revdepchecks are fine, this can be merged @krlmlr

@krlmlr
Copy link
Contributor

krlmlr commented Aug 29, 2024

I currently can't run revdepchecks...

@szhorvat
Copy link
Member Author

I currently can't run revdepchecks...

That's alright. Can we merge this though, even without doing the revdepchecks within the PR? We'd need to deal with revdepcheck breakages either way. Merging this now opens the door for exposing new functionality.

@krlmlr
Copy link
Contributor

krlmlr commented Sep 1, 2024

What functionality would you like to expose? Can we do it inside this PR? As long as it's only new features, it shouldn't make a difference for the revdepchecks.

It's easier from an organizational perspective to delay the merge until we can run the revdepchecks.

@krlmlr krlmlr force-pushed the try/c-core-2024-08-22 branch from c282139 to 20262b1 Compare September 1, 2024 03:28
@szhorvat
Copy link
Member Author

szhorvat commented Sep 1, 2024

Can we do it inside this PR?

Yes.

@szhorvat
Copy link
Member Author

szhorvat commented Sep 5, 2024

I'm going to have a look at these now.

@szhorvat
Copy link
Member Author

szhorvat commented Sep 5, 2024

I want to refactor R_check_int_scalar() and similar functions to be macros instead. This will allow error messages to point to the exact location of the failure in the sources. Any objection, @Antonov548 @krlmlr ?

@krlmlr
Copy link
Contributor

krlmlr commented Sep 5, 2024

Good idea re macros, no objections from me. What's the impact on the size of the compiled library?

@szhorvat
Copy link
Member Author

szhorvat commented Sep 5, 2024

Convenience link to revdepchecks README:

https://github.com/igraph/rigraph/blob/e04c35515df8ebfdfb7be49f94ee85d3f00d8b48/revdep/README.md

manynet

They pass a vector for the islands.size parameter of sample_islands(). This is invalid: it must be a scalar integer, and this has always been so.

I don't recall any recent changes in igraph that would cause this failure. We did add more stringent checks in 2.0.0, but that should have caught this misuse much earlier.

The fix needs to be done in the other package. EDIT: Notified, stocnet/manynet#86

MetaNet

???

Doesn't seem related to C core changes, I won't dig deeper at this time.

Should probably be investigated further.

netropy

They are passing NaN (or NA) values as edge weights when finding shortest path lengths. This has always been invalid.

Again, as I recall, the check for this was added a long time ago, not recently, so I am surprised that the failure appears only now.

EDIT: There seem to be no call to igraph::distances() in netropy. Hypothesis: the distance calculation might be done indirectly, through ggraph, potentially because it uses Kamada-Kawai, which then internally calls igraph_distances_dijksta(). I have not verified this. The change that triggers this failure may have been in a different package than netropy. All that said, using NA/NaN edge weights will be problematic/invalid with almost all igraph functions.

The fix needs to be done in the other package.

priorCON

Might need more diggint, but overall it looks like they expect a specific result for some sort of community detection (didn't check which one).

I did make a fix to igraph_community_multilevel() (cluster_louvain() in R) which causes it to return a different result for the same random seed. We do not guarantee the same result for the same seed across bugfix versions—this would prevent many kinds of bugfixes. We only guarantee the same statistical properties.

The fix needs to be done in the other package. EDIT: Notified, cadam00/priorCON#1

R6causal

???

Found the following significant warnings:
  Note: possible error in '.to(l[i])': unused argument (l[i]) 
  Note: possible error in '.from(l[i])': unused argument (l[i]) 

Does this ring a bell @maelle ? You were modifying these functions, as I recall.

simcausal

???

Doesn't look igraph related.

skynet

Looks like this is due to a deliberate deprecation, fix needs to be done in other package. CC @maelle

@szhorvat
Copy link
Member Author

szhorvat commented Sep 5, 2024

What's the impact on the size of the compiled library?

Probably very small, but needs to be measured, which should be easy to do. The checks themselves should add very little code. The large part is the error string, but the compiler consolidates duplicate strings.

@maelle
Copy link
Contributor

maelle commented Sep 5, 2024

Reg skynet ropensci/skynet#11, will also email a reminder

@krlmlr
Copy link
Contributor

krlmlr commented Sep 5, 2024

Only priorCON seems to be a new problem in this branch: f-revdep-2...try/c-core-2024-08-22

@krlmlr
Copy link
Contributor

krlmlr commented Sep 5, 2024

The other packages are also a problem in the main branch.

@schochastics
Copy link
Contributor

@szhorvat we are on it and will report back

@schochastics
Copy link
Contributor

Turns out there was a call to graph_from_adjacency where the input matrix contained NA weights on the lower.tri to indicate symmetry. This is being fixed in a pull request (termehs/netropy#4)

@szhorvat
Copy link
Member Author

szhorvat commented Sep 6, 2024

Thanks so much @schochastics and @termehs!

@termehs
Copy link

termehs commented Sep 6, 2024

Thanks all! New netropy version on its way to CRAN 😊

@szhorvat
Copy link
Member Author

@krlmlr I had to make a fairly crude change to the C core to deal with potentially incorrect results (see igraph/igraph#2674). I would like to request another round of revdepchecks for this PR. I'll talk to packages that are affected by this change. Hopefully there are not many.

@szhorvat
Copy link
Member Author

szhorvat commented Sep 12, 2024

We still have the weird issue that CI tests don't get run for C core updates. I did run R/igraph tests locally after this change and they pass.

@szhorvat
Copy link
Member Author

Can we merge this please (and have another revdepcheck round)?

@krlmlr
Copy link
Contributor

krlmlr commented Sep 18, 2024

Running checks now.

Copy link
Contributor

aviator-app bot commented Sep 18, 2024

This pull request failed to merge: PR cannot be automatically rebased, please rebase manually to continue. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Additional debug info: Failed to rebase this PR onto the latest changes from the base branch. You will probably need to rebase this PR manually and resolve conflicts).

@krlmlr
Copy link
Contributor

krlmlr commented Sep 18, 2024

No regressions, good to go. Thanks!

@krlmlr krlmlr force-pushed the try/c-core-2024-08-22 branch from d74de1b to c26ab44 Compare September 18, 2024 18:58
szhorvat and others added 5 commits September 18, 2024 20:59
…hese are exposed manually, and this is unlikely to change for a while
feat: allow NULL for cut_prob parameter of motif finding functions closes igraph/igraph#2555
docs: fix inaccuracy/typo in feedback_vertex_set() docs [skip ci]
fuzzer: update to libxml2 2.13.3 [skip ci]
chore: spelling in changelog [skip ci]
ci: temporarily disable ppc64 and s390x builds on Travis as the Travis infrastructure is currently broken
chore: bump cmake policy max
fix: fix type of result argument in igraph_feedback_vertex_set()
chore: update changelog
feat: igraph_feedback_vertex_set()
refactor: refer to igraph in a uniform way in headers, and minor code cleanup
doc: fix typo [skip ci]
chore: clarifying comment for cycle finding [skip ci]
refactor: work around GCC warning in feedback arc set implementation
chore: add missing copyright header to new feedback arc set test
ci: disable emulated checks on push; they can be started manually instead
ci: test on multiple architectures using emulation
refactor: further minor cleanup in feedback arc set
refactor: clean up Eades algorithm implementation for feedback arc set
refactor: rename IGRAPH_FAS_EXACT_IP_TO to IGRAPH_FAS_EXACT_IP_TI and expand docs
fix: feedback_arc_set() now disallows non-finite weights
chore: update changelog
feat: much faster exact minimum feedback arc set finding
feat: igraph_find_cycle()
feat: igraph_bitset_update()
chore: update changelog [skip ci]
feat: igraph_vector_reverse_section() and igraph_vector_rotate_left()
feat: igraph_strvector_swap_elements()
fix: the documented igraph_strvector_resize_min() was missing from headers
fix: memory cleanup order in Pajek reader
chore: code formatting [skip ci]
fix: Pajek reader warns about duplicate vertex IDs, fixes igraph/igraph#2416
refactor: minor cleanup in DIMACS flow writer
chore: update changelog [skip ci]
refactor: rename vector_contains() parameter in header as well
refactor: rename parameter in vector_contains() for consistency
deprecate: `igraph_vector_binsearch2()` is deprecated in favour of `igraph_vector_contains_sorted()`
deprecate: `igraph_vector_qsort_ind()` is deprecated in favour of `igraph_vector_sort_ind()`, closes igraph/igraph#2616
ci: more colour
fix: remove macros referencing non-existent vector_list_t types, refs igraph/igraph#2662
refactor: clean up igraph_compose()
test: test igraph_malloc()
test: memory allocation tests
doc: clean up memory allocation docs, fix non-included section
refactor: make variables more local in UMAP
fix: restore functionality of UMAP_DEBUG
chore: update changelog
fix: add missing error check in UMAP
fix: properly center UMAP layout in 3D
refactor: readability cleanup in UMAP code
fix: crash in igraph_layout_umap() when passing distances == NULL and distance_are_weights == true
refactor: adjlist_has_edge / adjlist_replace_edge cleanup
refactor: igraph_set_t formatting nitpicks
refactor: better error message in rewire()
refactor: more cleanup in fr layout
fuzzing: add misc_algos_weighted fuzzer which was already on the develop branch
refactor: readability and header fix in umap implementation
refactor: readability in kk and fr layout implementations
doc: doc improvements for chung_lu and watts_strogatz
chore: fix typos in changelog [skip ci]
doc: document vector_is_equal() ref igraph/igraph#2653
deprecate: igraph_array3_t
refactor: config header cleanup
ci: attempt to fix manual branch selection for coverity run again
test: fix memory leak in igraph_modularity() test
ci: fix manual selection of branch for coverity workflow
fix: memory leak introduced with previous fixes
chore: update changelog
chore: fix typo in parameter name
fix: add missing error check
fix: null-dereference in community_optimal_modularity() when passing the null or singleton graph, requesting modularity, but not requesting membership
fix: null-dereference in igraph_community_voronoi() when membership == NULL and modularity != NULL
fix: igraph_modularity() no longer allows NULL for 'modularity' as this is pointless and wasn't working correctly anyway
doc: documentation nitpicks and standardization
chore: update changelog [skip ci]
fix: work around potential infinite loop in igraph_community_multilevel() by repeated re-randomization of the vertex processing order fixes igraph/igraph#2650
fuzzer: use libxml2 2.13.2 [skip ci]
doc: fix typos in shortest paths docs
chore: update changelog [skip ci]
bench: fix memory leak in igraph_tree_game() benchmark
bench: fix memory leak in induced_subgraph_edges() benchmark
bench: minor cleanup and copyright headers
bench: add Leiden, Louvain community detection benchmark
chore: fix typo in changelog [skip ci]
chore: fix copyright headers in bitset source files
doc: improve bitset docs
chore: update changelog [skip ci]
doc: update old reference to deprecated function
doc: some improved doc formatting
chore: stylistic nitpick [skip ci]
doc: fix misformatting in bitset docs that prevented the inclusion of a doc line
doc: remove mention of long-remove stack_ptr_t
ci: move coverity over to develop
refactor: deprecate igraph_minimum_spanning_tree_(prim|unweighted)()
chore: correct Zenodo DOI
chore: Delete CODE_OF_CONDUCT.md (igraph/igraph#2645)
doc: commnity detection doc polish
doc: fix typo  [skip ci]
interface: add default variant for Floyd-Warshall
chore: fix typo in docs [skip ci]
chore: improve changelog
fix: eigenvector centralization now always assumes scaled vertex-level centrality scores
doc: more cross-linking for harmonic centrality
doc: add reference to betweenness docs
ci: apt-get update to work around outdated index
ci: test with clang 20
doc: doc cleanup in centralization functions
refactor: minor cleanup / IDE-friendliness for community_to_membership()
ci: re-enable travis on ppc64 / s390x
@krlmlr krlmlr force-pushed the try/c-core-2024-08-22 branch from c26ab44 to d4b6899 Compare September 18, 2024 18:59
@aviator-app aviator-app bot merged commit c07844d into main Sep 18, 2024
11 checks passed
@aviator-app aviator-app bot deleted the try/c-core-2024-08-22 branch September 18, 2024 20:02
@szhorvat
Copy link
Member Author

Thanks @krlmlr! I am almost surprised that the last C core change didn't cause revdepcheck failures, but I won't complain about good news :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants