Skip to content

Added acyclic coloring #60

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

Merged
merged 25 commits into from
Aug 26, 2019
Merged

Added acyclic coloring #60

merged 25 commits into from
Aug 26, 2019

Conversation

pkj-m
Copy link
Contributor

@pkj-m pkj-m commented Aug 7, 2019

Allows user to color graph using acyclic coloring algorithm.
The main difference between acyclic coloring and ordinary distance-1 coloring is that in the case of acyclic coloring, on top of the regular distance-1 coloring condition, there is an added condition that all the cycles in the graph have atleast 3 colors present.

Example to illustrate the difference:
a6d1589c-4600-4d7e-9a4c-958ef9995943

TODO:

  • 1. clean the code
  • 2. add tests
  • 3. replace indexing of firstTreeToVisit with either edge type or find a more efficient way to identify the specific edge object instead of iterating over all edges

#29

pkj-m added 13 commits July 29, 2019 23:48
The find_edge method has been reverted back to the old version with a worst case running time of O(size of E) where E is the edge set of graph g. The primary reason is that while the function can be optimized by returning a new edge, the ambiguity between which vertex is the source and which one is the destination breaks the disjoint set function as `Edge 1 => 2` is treated differently compared to `Edge 2=>1`.
TODO: add tests for checking condition 2 of acyclic coloring (all cycles of size 3 or more should have atleast 3 colors)
@matbesancon
Copy link
Contributor

I want to approve this PR just for the artistic drawings, but I'll restrain myself

@ChrisRackauckas
Copy link
Member

Those renderings, A+.

@matbesancon
Copy link
Contributor

@pkj-m to be a pain in the detailed-oriented, I'd advise putting all functions and variables in snake_case, not camelCase as mergeTrees and forbiddenColors

A_graph = matrix2graph(_A, partition_by_rows)
color_graph(A_graph,alg)
end
abstract type SparseDiffToolsColoringAlgorithm <: ArrayInterface.ColoringAlgorithm end
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this abstract type needed? Maybe the supertype from ArrayInterface is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why we implemented it this way. Maybe @ChrisRackauckas can throw some light over it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Makes it not type piracy

pkj-m and others added 5 commits August 15, 2019 01:21
Co-Authored-By: Mathieu Besançon <[email protected]>
Co-Authored-By: Mathieu Besançon <[email protected]>
Co-Authored-By: Mathieu Besançon <[email protected]>
Co-Authored-By: Mathieu Besançon <[email protected]>
@matbesancon
Copy link
Contributor

@pkj-m some few more comments, then it looks good

pkj-m and others added 4 commits August 22, 2019 23:48
Co-Authored-By: Mathieu Besançon <[email protected]>
Co-Authored-By: Mathieu Besançon <[email protected]>
@pkj-m
Copy link
Contributor Author

pkj-m commented Aug 26, 2019

@ChrisRackauckas @matbesancon can we merge this now?

@matbesancon
Copy link
Contributor

matbesancon commented Aug 26, 2019 via email

@ChrisRackauckas ChrisRackauckas merged commit 64c50f5 into JuliaDiff:master Aug 26, 2019
@pkj-m pkj-m deleted the acyclic branch August 26, 2019 20:26
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.

3 participants