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

Algorithm Result object complete rewrite, Add betweeen centrality, Add Zaks Karate Club #1327

Merged
merged 51 commits into from
Oct 19, 2023

Conversation

Haaroon
Copy link
Contributor

@Haaroon Haaroon commented Oct 9, 2023

What changes were proposed in this pull request?

A complete rewrite of thr algorithm object,

  • now allows users to get based on vertex ref, name etc.
  • adds betweeness centrality algorithm
  • adds karate club graph to built in graph gens
  • added many missing functions to python and into the macros
  • fixed all the associated algorithms and all their tests in both rust and python
  • added new tests
  • i hate macros ❤️
  • Add betweeen centrality
  • Add Zaks Karate Club
  • returns vertexviews in the algo result instead when collected or queried
  • added extra cmp checks in vertex+vertexview, added debug+fmt prints to vertexview
  • added very very basic graph hashing

may not come with memory efficiency

Why are the changes needed?

it was becoming very messy

Does this PR introduce any user-facing change? If yes is this documented?

yes all methods should have docs

How was this patch tested?

python and rust tests

Issues

resolves #1129

Are there any further changes required?

i hope not

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Comparison is base (9df4985) 57.58% compared to head (44918c8) 58.01%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
+ Coverage   57.58%   58.01%   +0.42%     
==========================================
  Files         186      188       +2     
  Lines       20011    20362     +351     
==========================================
+ Hits        11523    11812     +289     
- Misses       8488     8550      +62     
Files Coverage Δ
python/src/lib.rs 4.76% <ø> (ø)
raphtory/src/algorithms/centrality/betweenness.rs 100.00% <100.00%> (ø)
...ory/src/algorithms/centrality/degree_centrality.rs 97.36% <100.00%> (+0.07%) ⬆️
raphtory/src/algorithms/centrality/hits.rs 100.00% <100.00%> (ø)
raphtory/src/algorithms/centrality/pagerank.rs 99.03% <100.00%> (-0.01%) ⬇️
...rithms/community_detection/connected_components.rs 99.41% <100.00%> (+0.02%) ⬆️
raphtory/src/algorithms/metrics/balance.rs 100.00% <100.00%> (ø)
raphtory/src/algorithms/metrics/reciprocity.rs 98.76% <100.00%> (+0.01%) ⬆️
...lgorithms/motifs/three_node_local_single_thread.rs 98.74% <100.00%> (+0.04%) ⬆️
...ry/src/algorithms/pathing/temporal_reachability.rs 93.82% <100.00%> (+0.18%) ⬆️
... and 16 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Haaroon Haaroon marked this pull request as ready for review October 12, 2023 18:31
@Haaroon Haaroon changed the title Vertex Names Returned in New Algorithm Object Algorithm Result object complete rewrite Oct 12, 2023
Copy link
Collaborator

@miratepuffin miratepuffin left a comment

Choose a reason for hiding this comment

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

We need to go through some of these return types and extra for loops we have added as I think they are adding quite a bit of extra work. Can pair on this on monday.

raphtory/src/algorithms/metrics/balance.rs Outdated Show resolved Hide resolved
raphtory/src/algorithms/metrics/balance.rs Outdated Show resolved Hide resolved
raphtory/src/algorithms/algorithm_result.rs Outdated Show resolved Hide resolved
raphtory/src/algorithms/algorithm_result.rs Show resolved Hide resolved
Copy link
Collaborator

@miratepuffin miratepuffin left a comment

Choose a reason for hiding this comment

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

Loolks clean AF!
Couple of small suggestions for fixes + missing docs.
When we return via get_all can we add some small tests to check that the vertex has the correct views applied to it - i.e. if I run an algorithm on a windowed graph or a layered graph - when I get the result out it should also be windowed+layered.
Need to do benchmarking as discussed.

python/tests/test_graphdb.py Show resolved Hide resolved
raphtory/src/algorithms/algorithm_result.rs Show resolved Hide resolved
raphtory/src/algorithms/algorithm_result.rs Show resolved Hide resolved
raphtory/src/python/graph/algorithm_result.rs Outdated Show resolved Hide resolved
raphtory/src/python/graph/algorithm_result.rs Show resolved Hide resolved
raphtory/src/python/graph/algorithm_result.rs Outdated Show resolved Hide resolved
@Haaroon
Copy link
Contributor Author

Haaroon commented Oct 18, 2023

Loolks clean AF! Couple of small suggestions for fixes + missing docs. When we return via get_all can we add some small tests to check that the vertex has the correct views applied to it - i.e. if I run an algorithm on a windowed graph or a layered graph - when I get the result out it should also be windowed+layered. Need to do benchmarking as discussed.

added tests

@Haaroon
Copy link
Contributor Author

Haaroon commented Oct 18, 2023

FYI I was looking into our layered graphs, the layered graph do a layer on an edge_filter not a vertex filter. its assumed that vertices live on all layers, but their edges can be layered. Thus when running algorithms on a specific layer, you will get a result that contains all vertexes, as they exist in all layers but may not have an edge on that layer.

@Haaroon Haaroon merged commit b4eef2e into master Oct 19, 2023
13 checks passed
@Haaroon Haaroon deleted the feature/vertex-id-algo-v3 branch October 19, 2023 08:26
fabianmurariu pushed a commit that referenced this pull request May 21, 2024
…d Zaks Karate Club (#1327)

* checkpoint, need to fix tests

* im so happy it works :D

* remove type_name param

* algorithm result tests complete, new fmt, fixed group_by, fixed all types

* renamed old algorithm result, changed base result type to a hashmap instead of vector due to potential issues with sparse vectors if the dataset is large, added missing G type for the graph, brought back results type

* fix bad 0 checking

* macro magic but partially implemented

* added the extra macros, added all missing functions, removed all pointers for clones (:vomit: pyo3)

* algorithm result object + all macros is complete in both rust and python

* fixed reciprocity, fixed centrality

* fixed pagerank, fixed graphql new type issues

* balance algorithm complete

* sssp fixed

* sssp forgot le lib

* temporal reachability fixed

* hits fixed

* connected components nearly fixed, last test

* removed bad test

* three node motif completed, algorithm result complte

* good bye bugs

* removed all warnings

* fix lotr bug

* i think i fixed hulong

* fix dusty benchmark

* rename macro rules, fix doctests

* implement debug for algo result

* fix pytests

* Zaks Karate Club Graph (#1326)

* betweenness centrality for a directed graph with and without normalisation

* cleanup

* swapped to algorithm result, added rust docs

* ported to python, added python docs, added python test

* fix formatting

* karate club graph, but its half working, adding 1 extra node and way too many edges

* fixed issue with rows

* betweeness has float calc differences but largely the same

* fix test

* fixes issue with python?

* port betweeness centrality to new algo object and fix over python tests

* Changed State to use Internal ID everywhere, fixed most of the algorithms, fixed the tests

* fixed all the algos

* fn name changes

* added very basic hashing to allow get_all to return a vertex object

* vertex view now returned by algo rest

* fix python algo result for vertexview

* reorder tests

* implemented custom debug fmt display for vertex view, implemented custom display fmt, fixed algorithm tests, fixed all rust issues

* extended richcmp for python objects, fixed all python tests

* connected_components.rs passes

* moved algos

* resolved comments

* changes due to comments

* windows works, layers dont

* added tests for windowed graphs

* complete!

* bad comments for ubuntu

* bad colon
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.

Rewrite the algorithms to take in the Vertex Id objects instead of strings
2 participants