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

Work on greedy graph coloring. #94

Merged
merged 6 commits into from
Sep 17, 2023

Conversation

ndcroos
Copy link
Contributor

@ndcroos ndcroos commented Sep 11, 2023

See #88.

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage is 98.68% of modified lines.

Files Changed Coverage
...aflib/algorithm/coloring/greedy_graph_coloring.tpp 92.30%
test/graaflib/algorithm/graph_coloring_test.cpp 100.00%

📢 Thoughts on this report? Let us know!.

@ndcroos
Copy link
Contributor Author

ndcroos commented Sep 11, 2023

I removed the empty dir 'coloring' and renamed 'greedy_graph_coloring_test.cpp' to 'graph_coloring_test.cpp'
If a separate dir (coloring) is used for contain different files with test cases (to reflect the directory structure in the include dir), it seems that it won't execute the tests in these files without other changes.

Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, looks very good already!
Mostly some style related feedback

README.md Outdated Show resolved Hide resolved
docs/docs/algorithms/coloring/greedy-graph-coloring.md Outdated Show resolved Hide resolved
docs/docs/algorithms/coloring/greedy-graph-coloring.md Outdated Show resolved Hide resolved
@bobluppes
Copy link
Owner

I removed the empty dir 'coloring' and renamed 'greedy_graph_coloring_test.cpp' to 'graph_coloring_test.cpp' If a separate dir (coloring) is used for contain different files with test cases (to reflect the directory structure in the include dir), it seems that it won't execute the tests in these files without other changes.

Thanks for reporting! Sounds like I will have to adjust the CMake setup in order for this to work. I will do so in a follow-up

@bobluppes bobluppes added the enhancement New feature label Sep 17, 2023
@bobluppes
Copy link
Owner

closes #88

Copy link
Owner

@bobluppes bobluppes left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback, LGTM 🎉

@bobluppes bobluppes merged commit f1463eb into bobluppes:main Sep 17, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants