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

Add regular tree generator #197

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

stecrotti
Copy link

@stecrotti stecrotti commented Nov 23, 2022

Generator for a regular tree, also known as Bethe Lattice or Cayley Tree, which generalizes a binary tree to arbitrary degree.

I tried to keep the code as close as possible to the one for binary_tree.

@stecrotti stecrotti marked this pull request as ready for review November 23, 2022 15:28
@codecov
Copy link

codecov bot commented Nov 24, 2022

Codecov Report

Merging #197 (b26cf91) into master (ea6bcfe) will decrease coverage by 0.13%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   97.40%   97.26%   -0.14%     
==========================================
  Files         109      109              
  Lines        6468     6506      +38     
==========================================
+ Hits         6300     6328      +28     
- Misses        168      178      +10     

@stecrotti stecrotti marked this pull request as draft November 24, 2022 12:52
Comment on lines 556 to 557
k <= 0 && return SimpleGraph(0)
k == 1 && return SimpleGraph(1)
Copy link
Member

Choose a reason for hiding this comment

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

In these cases we return a graph of of eltype Int, but in all other cases we return a graph of type T.

Copy link
Member

Choose a reason for hiding this comment

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

To be completely honest, I am not sure if it is the best idea to determine the eltype of the graphs from k and z - we do that indeed for other graphs here, but I never really liked that.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can have a function signature

regular_tree(T::Type{<:Integer}, k::Integer, z::Integer)

and then define something like

regular_tree(k::Integer, z::Integer) = regular_tree(Int64, k, z)

that for be similar to the function sprand from SparseArrays for example.

"""
regular_tree(k::Integer, z::integer)

Create a [regular tree](https://en.wikipedia.org/wiki/Bethe_lattice),
Copy link
Member

Choose a reason for hiding this comment

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

That wikipedia article might be a bit confusing, because it talks about infinite trees in the introduction.

Maybe we can decribe that a bit more clearly here?

If I understood correctly, what you want to create is a perfect k-ary tree such as described here? https://en.wikipedia.org/wiki/M-ary_tree#Types_of_m-ary_trees

@stecrotti
Copy link
Author

By the way I called it 'regular tree' because of the analogy with regular graphs but that might not be too accurate because:

  1. A regular tree is not a regular graph (because of the leaves)
  2. z-regular graphs have nodes of degree z while z-ary trees have nodes of degree z+1, potentially confusing

That said, mary_tree(k, m), zary_tree(k, z) don't look great to me.

@stecrotti
Copy link
Author

Changes:

  • New method with type regular_tree(T::Type{<:Integer}, k::Integer, z::Integer)
  • Improved docs with correct wiki page
  • InexactError instead of DomainError when number of vertices is not representable
  • Assert z > 0, fallback to a path graph in case z = 1
  • Appropriate conversions to the provided type T, checked by test

@stecrotti stecrotti marked this pull request as ready for review November 30, 2022 09:47
@simonschoelly
Copy link
Member

By the way I called it 'regular tree' because of the analogy with regular graphs but that might not be too accurate because:

1. A regular tree is not a regular graph (because of the leaves)

2. z-regular graphs have nodes of degree z while z-ary trees have nodes of degree z+1, potentially confusing

That said, mary_tree(k, m), zary_tree(k, z) don't look great to me.

I think regular tree is perfectly fine. For example here, the same term is used: https://www.wolframalpha.com/examples/mathematics/discrete-mathematics/graph-theory/regular-trees/

Copy link
Member

@aurorarossi aurorarossi left a comment

Choose a reason for hiding this comment

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

Some comments on the docstrings. If you agree with the style then I will open a PR to fix double_binary_tree and binary_tree

@@ -536,6 +536,60 @@ function double_binary_tree(k::Integer)
return g
end

"""
regular_tree([T=Int64, ], k::Integer, z::integer)
Copy link
Member

Choose a reason for hiding this comment

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

Except for binary_tree and double_binary_tree all the other functions of this file have the following style:

Suggested change
regular_tree([T=Int64, ], k::Integer, z::integer)
regular_tree(T, k, z)

Comment on lines +548 to +550
julia> regular_tree(4, 3)
{40, 39} undirected simple Int64 graph

Copy link
Member

Choose a reason for hiding this comment

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

I would remove it and I would put it in the docstring of the function regular_tree(k,z).

Suggested change
julia> regular_tree(4, 3)
{40, 39} undirected simple Int64 graph

Comment on lines +553 to +555

julia> regular_tree(5, 2) == binary_tree(5)
true
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

Suggested change
julia> regular_tree(5, 2) == binary_tree(5)
true

end
return SimpleGraph(ne, fadjlist)
end

Copy link
Member

Choose a reason for hiding this comment

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

I would add the docstring also to this function.

Suggested change
"""
regular_tree(k, z)
Create a regular tree or [perfect z-ary tree](https://en.wikipedia.org/wiki/M-ary_tree#Types_of_m-ary_trees):
a `k`-level tree where all nodes except the leaves have exactly `z` children.
For `z = 2` one recovers a binary tree.
# Examples
```jldoctest
julia> regular_tree(4, 3)
{40, 39} undirected simple Int64 graph
julia> regular_tree(5, 2) == binary_tree(5)
true
```
"""

Copy link
Member

Choose a reason for hiding this comment

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

It's quite often the case, that the docstrings for multiple methods of a function are only added for a single method - I think that would also be better here, than adding docstrings for both methods of this function.

@dapias
Copy link

dapias commented Dec 14, 2022

I just realized that a $k$-ary tree is not the same as a Cayley tree of connectivity $k$ because in a $k$-ary tree, the root has connectivity $k$, and all the nodes excepting those in the leaves have connectivity $k+1$. Look, for instance, at the attached plot, nodes 2,3 and 4 have connectivity four and not three as it would be the case for a Cayley tree

Bildschirm­foto 2022-12-14 um 09 31 30

@stecrotti
Copy link
Author

Agree! Indeed it was corrected and should be ok in the current version of this PR

@dapias
Copy link

dapias commented Dec 15, 2022

Is there a function to create an actual Cayley tree?

@stecrotti
Copy link
Author

Not that i know of

@dapias
Copy link

dapias commented Dec 16, 2022

I am not an expert, but it seems trivial to be implemented, given the actual code for the tree. After setting up the central node (root), one only needs to demand that each child has $(z-1)$ children instead of $z$

```
"""
function regular_tree(T::Type{<:Integer}, k::Integer, z::Integer)
z <= 0 && throw(DomainError(z, "number of children must be positive"))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also check for k >= 0 here?

Comment on lines 563 to 567
if Graphs.isbounded(k) && (BigInt(z)^k - 1) ÷ (z - 1) > typemax(T)
throw(InexactError(:convert, T, (BigInt(z)^k - 1) ÷ (z - 1)))
end

n = T((z^k - 1) / (z - 1))
Copy link
Member

Choose a reason for hiding this comment

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

You already calculate the number of vertices with (BigInt(z)^k - 1) ÷ (z - 1), so we we could just reuse this for n.

With the expression n = T((z^k - 1) / (z - 1)) there are two issues:

  • z^k might overflow before converting it to T
  • By using the / operator instead of ÷ your intermediate result will be a double

Copy link
Member

Choose a reason for hiding this comment

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

A lot of these overflow issues probably won't happen if we restrict k and z to Int in the function definition.

@stecrotti
Copy link
Author

Changed the signature to match the rest of the file as suggested by @aurorarossi, but kept only one docstring.

Fixed potential overflow as suggested by @simonschoelly.

@simonschoelly
Copy link
Member

There is this one comment from me, about k >= 0 - not sure if you have seen it, but feel free to ignore that it you think we should not change anything there.

Otherwise looks good for me - I did not check the math, but choose to believe the tests.

@stecrotti
Copy link
Author

Sure, i replied under your review comment:

I stuck to what was used in the rest of the file which is that for k<=0 it returns a SimpleGraph(zero(T)). I guess there was a good reason for that? If not it makes total sense to error if k<0.

@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants