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

fix(analytical): fix the louvain algorithm that the result is not consistent with #183 #3472

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

acezen
Copy link
Collaborator

@acezen acezen commented Jan 8, 2024

What do these changes do?

This change try to fix the louvain algorithm that the result is not consistent with #183

Related issue number

Close #3457

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f0441bb) 40.90% compared to head (f9a7e4d) 70.96%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #3472       +/-   ##
===========================================
+ Coverage   40.90%   70.96%   +30.06%     
===========================================
  Files         109      110        +1     
  Lines       11343    11384       +41     
===========================================
+ Hits         4640     8079     +3439     
+ Misses       6703     3305     -3398     
Files Coverage Δ
python/graphscope/tests/unittest/test_app.py 98.00% <100.00%> (+98.00%) ⬆️
python/graphscope/tests/unittest/test_lazy.py 93.66% <100.00%> (+93.66%) ⬆️

... and 63 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0441bb...f9a7e4d. Read the comment docs.

@JackyYangPassion
Copy link
Contributor

JackyYangPassion commented Jan 8, 2024

unit test #3474

expect 103 community but 10733

FAILED ../../../.local/lib/python3.8/site-packages/graphscope/tests/unittest/test_app.py::test_other_app_on_undirected_graph - assert 10733 == 103

@acezen
Copy link
Collaborator Author

acezen commented Jan 9, 2024

unit test #3474

expect 103 community but 10733

FAILED ../../../.local/lib/python3.8/site-packages/graphscope/tests/unittest/test_app.py::test_other_app_on_undirected_graph - assert 10733 == 103

Yes, there are still something wrong and it's not related to min_progress parameter, trying to figure out

@acezen
Copy link
Collaborator Author

acezen commented Jan 11, 2024

unit test #3474

expect 103 community but 10733

FAILED ../../../.local/lib/python3.8/site-packages/graphscope/tests/unittest/test_app.py::test_other_app_on_undirected_graph - assert 10733 == 103

I find the real problem for that, it seems that we need to do a wcc to the louvain result and then get the real community result... since the community id in graphscope can represent by any of the id in the community.

@JackyYangPassion
Copy link
Contributor

unit test #3474
expect 103 community but 10733

FAILED ../../../.local/lib/python3.8/site-packages/graphscope/tests/unittest/test_app.py::test_other_app_on_undirected_graph - assert 10733 == 103

I find the real problem for that, it seems that we need to do a wcc to the louvain result and then get the real community result... since the community id in graphscope can represent by any of the id in the community.

Very cool. @acezen Can you provide the logic for subsequent processing? I can verify it based on test data. I have been watching GAE for the past two days based on the PIE model, trying to find the root cause in a difficult way.

@acezen
Copy link
Collaborator Author

acezen commented Jan 11, 2024

unit test #3474
expect 103 community but 10733

FAILED ../../../.local/lib/python3.8/site-packages/graphscope/tests/unittest/test_app.py::test_other_app_on_undirected_graph - assert 10733 == 103

I find the real problem for that, it seems that we need to do a wcc to the louvain result and then get the real community result... since the community id in graphscope can represent by any of the id in the community.

Very cool. @acezen Can you provide the logic for subsequent processing? I can verify it based on test data. I have been watching GAE for the past two days based on the PIE model, trying to find the root cause in a difficult way.

later I will push a commit that output the correct result of Lovain, then it should not need user to do the union find.

@acezen
Copy link
Collaborator Author

acezen commented Jan 12, 2024

hi, @shirly121, the test_gremlin_timeout failed in CI, can you help look at the reason for the failure?
https://github.com/alibaba/GraphScope/actions/runs/7496630317/job/20409902909?pr=3472

siyuan0322
siyuan0322 previously approved these changes Jan 15, 2024
@acezen acezen merged commit d9db184 into alibaba:main Jan 15, 2024
30 checks passed
@acezen acezen deleted the 3457-fix-louvain branch January 15, 2024 08:25
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.

[BUG] Run louvain algo on k8s error
5 participants