Skip to content

Document fields of TreeSet #296

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.

Sign up for GitHub

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
amontoison merged 5 commits into from
Jan 29, 2026
Merged

Document fields of TreeSet #296

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ce6e42
Document fields of TreeSet
blegat Jan 9, 2026
8b377be
Update src/coloring.jl
blegat Jan 10, 2026
d25581b
Update src/coloring.jl
blegat Jan 10, 2026
4cd18b5
Address comments
blegat Jan 19, 2026
20ab117
Fix
blegat Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/coloring.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,16 +422,26 @@ Encode a set of 2-colored trees resulting from the [`acyclic_coloring`](@ref) al
$TYPEDFIELDS
"""
struct TreeSet{T}
"""
contains the reverse breadth first (BFS) traversal order for each tree in the forest.
More precisely, given an edge `(u, v)` of index `i`,
`reverse_bfs_order[i]` is either `(u, v)` or `(v, u)`.
Copy link
Copy Markdown
Collaborator

@amontoison amontoison Jan 9, 2026

Choose a reason for hiding this comment

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

It is not right, i is not an edge index.

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 19, 2026

Choose a reason for hiding this comment

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

Should be fixed now

The first node of the tuple is the leaf in the reverse BFS order.
"""
reverse_bfs_orders::Vector{Tuple{T,T}}
"For a tree index `1 <= k <= nt`, `is_star[k]` indicates whether the `k`th three is a star."
is_star::Vector{Bool}
"`tree_edge_indices[1]` is one and `tree_edge_indices[k+1] - tree_edge_indices[k]` is the number of edges in the `k`th tree"
Comment thread
blegat marked this conversation as resolved.
Outdated
Comment thread
blegat marked this conversation as resolved.
Outdated
tree_edge_indices::Vector{T}
"numbers of 2-colored trees for which trees sharing the same 2 colors have disjoint vertices"
Copy link
Copy Markdown
Collaborator

@amontoison amontoison Jan 9, 2026

Choose a reason for hiding this comment

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

I don't think they have disjoint vertices. When the matrix is dense, we have n(n+1)/2 trees with two vertices (one for each edge) but only n vertices.

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 10, 2026

Choose a reason for hiding this comment

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

If a red vertex u is adjacent to both a blue and yellow vertex then the red-blue tree and red-yellow tree have the u vertex in common so they don't have disjoint vertices

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 19, 2026

Choose a reason for hiding this comment

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

Ahah, I contradicted myself in my reply. I meant disjoint edges indeed

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 19, 2026

Choose a reason for hiding this comment

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

Fixed it in the last commit

nt::T
end

function TreeSet(
g::AdjacencyGraph{T},
forest::Forest{T},
buffer::AbstractVector{T},
# The value of `reverse_bfs_orders` is ignored, we just provide the storage for it (or reuse memory allocated during acyclic coloring)
reverse_bfs_orders::Vector{Tuple{T,T}},
ne::Integer,
) where {T}
Expand Down Expand Up @@ -566,7 +576,6 @@ function TreeSet(
# Number of edges treated
num_edges_treated = zero(T)

# reverse_bfs_orders contains the reverse breadth first (BFS) traversal order for each tree in the forest
Copy link
Copy Markdown
Collaborator

@amontoison amontoison Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We should keep this comment.
What do think of:

# reverse_bfs_orders will contain the reverse breadth first (BFS) traversal order for each tree in the forest concatenated

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 10, 2026

Choose a reason for hiding this comment

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

I simply moved it into the comment of the field of the struct. I feel that the definition of what reverse_bfs_orders will be should go there and here we should comment on why what we do achieve this

Copy link
Copy Markdown
Contributor Author

@blegat blegat Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I added a new comment clarifying the confusion with the root of the Forest union-find datastructure that we thought we be useful during our discussion on the phone

for k in 1:nt
# Initialize the queue to store the leaves
queue_start = 1
Expand Down
Loading