Skip to content

Conversation

@guru2396
Copy link
Contributor

This PR is for Issue #340 .

  • Caching the adjacency matrix of the graph when graph is created.
  • Modifying the cached adjacency matrix when edge is added/removed.
  • Performing faster edge lookup using cached adjacency matrix.

@github-actions github-actions bot added test Something about test core something about core labels Sep 14, 2023
@ghost
Copy link

ghost commented Sep 14, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #344 (d3815b9) into master (43e3830) will increase coverage by 0.09%.
The diff coverage is 92.55%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   97.31%   97.41%   +0.09%     
==========================================
  Files          55       55              
  Lines        8455     8539      +84     
==========================================
+ Hits         8228     8318      +90     
+ Misses        227      221       -6     
Flag Coverage Δ
unittests 97.41% <92.55%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
include/Graph/Graph.hpp 95.36% <88.70%> (+0.49%) ⬆️
test/GraphTest.cpp 99.82% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

@ZigRazor ZigRazor requested a review from sbaldu September 20, 2023 10:04
@nrkramer nrkramer changed the title Caching Adjacency Matrix for faster edge lookup. Caching Adjacency Matrix for faster edge lookup Sep 23, 2023
}) != nodeSet.end());
}

TEST(GraphTest, FindEdge_Test) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add spacing in logical places between lines in this test. It's hard to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Please review.

Copy link
Collaborator

@nrkramer nrkramer left a comment

Choose a reason for hiding this comment

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

Looks great so far!

Only one nit from me to maybe rename getCacheAdjMatrix() to cacheAdjMatrix()

Copy link
Collaborator

@nrkramer nrkramer left a comment

Choose a reason for hiding this comment

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

All my comments have been addressed. Thanks!

@sbaldu
Copy link
Collaborator

sbaldu commented Sep 24, 2023

Looks good to me as well

@ZigRazor ZigRazor linked an issue Sep 25, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core something about core test Something about test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Faster edge lookup

4 participants