Skip to content

Partitioning HDRF and Other - #137

Merged
ZigRazor merged 8 commits into
masterfrom
dev
Jan 19, 2022
Merged

ZigRazor merged 8 commits into
masterfrom
dev

Conversation

@ZigRazor

Copy link
Copy Markdown
Owner

Partitioning HDRF and Other algorithm, with general mechanism

(TODO check how to use CoordinatedRecord to clean memory )

Signed-off-by: ZigRazor <zigrazor@gmail.com>
@ZigRazor ZigRazor added enhancement New feature or request development Development of new Functionalities core something about core labels Oct 20, 2021
@ZigRazor ZigRazor self-assigned this Oct 20, 2021
@github-actions github-actions Bot added the test Something about test label Oct 20, 2021
@ghost

ghost commented Oct 20, 2021

Copy link
Copy Markdown

CodeSee Review Map:

Review these changes using an interactive CodeSee Map

Review in an interactive map

View more CodeSee Maps

Legend

CodeSee Map Legend

Corrected crash on print out Graph and Partition
Corrected test
Corrected Thread Safety for CoordinatePartitionState

Signed-off-by: ZigRazor <zigrazor@gmail.com>
@codecov-commenter

codecov-commenter commented Oct 25, 2021

Copy link
Copy Markdown

Codecov Report

Merging #137 (d65dd75) into master (dce00a7) will increase coverage by 0.11%.
The diff coverage is 91.66%.

❗ Current head d65dd75 differs from pull request most recent head 82a4a0c. Consider uploading reports for the commit 82a4a0c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   97.15%   97.26%   +0.11%     
==========================================
  Files          41       41              
  Lines        4748     4647     -101     
==========================================
- Hits         4613     4520      -93     
+ Misses        135      127       -8     
Flag Coverage Δ
unittests 97.26% <91.66%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
include/Partitioning/CoordinatedRecord.hpp 73.91% <ø> (-2.09%) ⬇️
include/Partitioning/Partition.hpp 80.00% <ø> (+6.31%) ⬆️
test/main.cpp 100.00% <ø> (ø)
include/Partitioning/CoordinatedPartitionState.hpp 61.19% <50.00%> (-4.38%) ⬇️
include/Partitioning/HDRF.hpp 82.35% <85.71%> (+11.08%) ⬆️
include/Graph/Graph.hpp 98.11% <100.00%> (+0.07%) ⬆️
test/PartitionTest.cpp 100.00% <100.00%> (ø)
test/Utilities.hpp 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

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

@lgtm-com

lgtm-com Bot commented Oct 25, 2021

Copy link
Copy Markdown

This pull request introduces 1 alert and fixes 1 when merging d65dd75 into 6fb620a - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor

fixed alerts:

  • 1 for Resource not released in destructor

Added also tests for this algorithm.

Signed-off-by: ZigRazor <zigrazor@gmail.com>
@ZigRazor
ZigRazor requested review from AlfredCP and sidml and removed request for sidml October 28, 2021 09:33
@ZigRazor
ZigRazor requested review from sidml and removed request for AlfredCP and sidml October 28, 2021 09:34
srand(time(NULL));

int choice = rand() % candidates.size();
unsigned int seed = (unsigned int)time(NULL);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see a lot of C-style casting. Is there any strong reason not to user C++ static_cast<> syntax? It is safer.

This can be a big change, so it can be open in another ticket

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes, I think can be a good issue

int CoordinatedPartitionState<T>::getTotalReplicas()
{
//TODO
std::lock_guard<std::mutex> lock(*record_map_mutex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am pro "const" wherever we can and even more for critical things, like mutex, thread-related objects etc.
Then we could say that the code is more protected from unexpected modification related bugs, detected in compiling time

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

yes, I agree

@AlfredCP AlfredCP left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ZigRazor , I added a couple of comments about using "const" anc C++ casting style.
Both observations are motivated for having a safer code from unexpected errors/bugs which normally are way harder to find/debug

Signed-off-by: ZigRazor <zigrazor@gmail.com>
Signed-off-by: ZigRazor <zigrazor@gmail.com>
@ZigRazor
ZigRazor marked this pull request as ready for review January 19, 2022 21:40
@github-actions github-actions Bot added the repo something about repo label Jan 19, 2022
@ZigRazor
ZigRazor merged commit b168640 into master Jan 19, 2022
@lgtm-com

lgtm-com Bot commented Jan 19, 2022

Copy link
Copy Markdown

This pull request introduces 3 alerts and fixes 1 when merging 82a4a0c into e735fc6 - view on LGTM.com

new alerts:

  • 2 for Resource not released in destructor
  • 1 for Catching by value

fixed alerts:

  • 1 for Resource not released in destructor

@ZigRazor
ZigRazor deleted the dev branch January 19, 2022 22:10
@ZigRazor ZigRazor linked an issue Jan 27, 2022 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 development Development of new Functionalities enhancement New feature or request repo something about repo test Something about test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implementation of HDRF Graph Partitioning

3 participants