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 BananaTree #411

Merged
merged 30 commits into from
Mar 17, 2021
Merged

Add BananaTree #411

merged 30 commits into from
Mar 17, 2021

Conversation

marinaanagno
Copy link
Contributor

No description provided.

@marinaanagno
Copy link
Contributor Author

@LRacine @samanthaharper

doc/examples.xml Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
@marinaanagno
Copy link
Contributor Author

@alex-konovalov thank you so much for your comments! Fixed them just now! :)

@james-d-mitchell james-d-mitchell added the new-feature A label for new features. label Mar 1, 2021
doc/examples.xml Outdated Show resolved Hide resolved
flsmith
flsmith previously approved these changes Mar 3, 2021
Copy link
Collaborator

@flsmith flsmith left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things

doc/examples.xml Outdated Show resolved Hide resolved
gap/examples.gi Show resolved Hide resolved
Copy link
Contributor

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Please fix spurious character in line 1

Marina Anagnostopoulou-Merkouri added 2 commits March 7, 2021 16:51
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Something's gone wrong with this PR. There is a new empty file oper.gi that shouldn't exist, and there is a new test file tst/standard/\ that is probably an unintended duplicate of tst/standard/examples.tst, and moreover within tst/standard/examples.tst, the tests for BananaTreeGraph should appear before the DIGRAPHS_StopTest() and STOP_TEST lines.

@marinaanagno
Copy link
Contributor Author

Thank you @wilfwilson! I'm really sorry, I have no idea how I managed to do this, I'll try and fix it now! :)

@wilfwilson
Copy link
Collaborator

@marinaanagno I just saw that I can delete those files myself through the GitHub interface, so I've done that now. Hopefully this will make the tests pass!

wilfwilson
wilfwilson previously approved these changes Mar 10, 2021
@wilfwilson wilfwilson self-requested a review March 10, 2021 12:57
@wilfwilson wilfwilson dismissed their stale review March 10, 2021 12:57

Accidental premature approval, sorry!

@wilfwilson wilfwilson removed their request for review March 10, 2021 12:58
@marinaanagno
Copy link
Contributor Author

@wilfwilson that would be awesome if you have a second at some point! I'm so sorry for being such a pain today, I don't know how I messed this up :)

Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Hopefully this will be my last requested changes before merging!

doc/examples.xml Outdated Show resolved Hide resolved
tst/standard/examples.tst Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
@wilfwilson
Copy link
Collaborator

@wilfwilson that would be awesome if you have a second at some point! I'm so sorry for being such a pain today, I don't know how I messed this up :)

Happy to help whenever, just send me a message and we can do some screen sharing if you think that would help.

marinaanagno and others added 5 commits March 10, 2021 20:14
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
@digraphs digraphs deleted a comment from codecov bot Mar 11, 2021
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

This is looking good, thanks for bearing with me with all the changes that I've asked for.

Unfortunately I have a few more! Having reviewed some of the other PRs, I've realised a few things that I want to look out for. I also noticed the following error that your code can run into:

gap> BananaTreeGraph(2,1);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CompleteBipartiteDigraph' on 3 argument\
s at /Users/Wilf/gap/lib/methsel2.g:249 called from
CompleteBipartiteDigraph( IsMutable, 1, n - 1
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:381 called from
BananaTreeGraphCons( IsMutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:394 called from
BananaTreeGraphCons( IsImmutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:403 called from
<function "BananaTreeGraph for a function and two pos ints">( <arguments> )
 called from read-eval loop at *stdin*:4
type 'quit;' to quit to outer loop

There are also now merged conflicts between this PR and the master branch (because PRs #424 and #425 were merged). Depending on your experience level with git, this might be a bit tricky to resolve. Let me know if you have problems and I'll be happy to help out, or to just resolve the conflicts for you.

doc/examples.xml Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
doc/examples.xml Outdated
If the optional first argument <A>filt</A> is not present, then <Ref
Filt="IsImmutableDigraph"/> is used by default.<P/>

See also <Ref Oper="CompleteBipartiteDigraph"/>.
Copy link
Collaborator

@wilfwilson wilfwilson Mar 11, 2021

Choose a reason for hiding this comment

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

If possible, would you mind being more explicit about the correspondence between the abstract definition and the actual vertices/edges that this function gives you, analogous to what I suggested in https://github.com/digraphs/Digraphs/pull/423/files#r592305981

In particular, I think this documentation should note that vertex 1 is that single root vertex, and that the copies of the stars live on the vertices [2 .. n+1], [n+2 .. 2*n+1], ... etc, and also which vertex of each star is connected to 1.

@marinaanagno
Copy link
Contributor Author

This is looking good, thanks for bearing with me with all the changes that I've asked for.

Unfortunately I have a few more! Having reviewed some of the other PRs, I've realised a few things that I want to look out for. I also noticed the following error that your code can run into:

gap> BananaTreeGraph(2,1);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CompleteBipartiteDigraph' on 3 argument\
s at /Users/Wilf/gap/lib/methsel2.g:249 called from
CompleteBipartiteDigraph( IsMutable, 1, n - 1
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:381 called from
BananaTreeGraphCons( IsMutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:394 called from
BananaTreeGraphCons( IsImmutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:403 called from
<function "BananaTreeGraph for a function and two pos ints">( <arguments> )
 called from read-eval loop at *stdin*:4
type 'quit;' to quit to outer loop

There are also now merged conflicts between this PR and the master branch (because PRs #424 and #425 were merged). Depending on your experience level with git, this might be a bit tricky to resolve. Let me know if you have problems and I'll be happy to help out, or to just resolve the conflicts for you.

Thank you so much for all your help @wilfwilson! I will make the changes probably this evening. I have solved a merge conflict manually once or twice, but if there's some other git method that does it differently I'd be very glad to learn about it! :)

@wilfwilson wilfwilson changed the title Add banana tree Add BananaTree Mar 11, 2021
Marina Anagnostopoulou-Merkouri added 2 commits March 12, 2021 15:46
@marinaanagno
Copy link
Contributor Author

@wilfwilson git fetch and merge didn't quite work for me (obviously because I did something the wrong way) and I wasn't getting the changes from master. I used git rebase and hopefully I managed to work my way through it. The tests might fail miserably. I will make the rest of the changes now :)

Copy link
Collaborator

@wilfwilson wilfwilson 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 doing those changes. I'd still like for you to explicitly say what the banana tree is, i.e. #411 (comment).

I pushed a couple of tweaks, including I changed (389b147) the variables in the documentation to be n and k, rather than m and n. You were quoting Wolfram Alpha, and putting that sentence in <Q> tags (i.e. quote marks), but you weren't actually exactly quoting. Now the text is the same.

I've also found a bug. The question is: do you want to support the second argument being 1 or not? If so, then this error shouldn't happen. If not, then it requires a proper error (and documentation) stating that the second argument must be at least 2.

gap> BananaTree(1, 1);
Error, the 2nd argument <ran> must be a vertex of the digraph <D> that is the 1st argument, at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:184 called from
DigraphAddEdge( D, edge[1], edge[2]
 ) at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:206 called from
DigraphAddEdge( D, edge ); at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:218 called from
DigraphAddEdges( D, [ [ 1, j * n + 3 ], [ j * n + 3, 1 ] ]
 ); at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:617 called from
BananaTreeCons( IsMutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:627 called from
BananaTreeCons( IsImmutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:636 called from
...  at *stdin*:1
type 'quit;' to quit to outer loop
gap> BananaTree(2, 1);
Error, the 2nd argument <ran> must be a vertex of the digraph <D> that is the 1st argument, at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:184 called from
DigraphAddEdge( D, edge[1], edge[2]
 ) at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:206 called from
DigraphAddEdge( D, edge ); at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:218 called from
DigraphAddEdges( D, [ [ 1, j * n + 3 ], [ j * n + 3, 1 ] ]
 ); at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:617 called from
BananaTreeCons( IsMutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:627 called from
BananaTreeCons( IsImmutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:636 called from
...  at *stdin*:1
type 'quit;' to quit to outer loop
gap> BananaTree(10,1);
Error, the 2nd argument <ran> must be a vertex of the digraph <D> that is the 1st argument, at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:184 called from
DigraphAddEdge( D, edge[1], edge[2]
 ) at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:206 called from
DigraphAddEdge( D, edge ); at /Users/Wilf/gap/pkg/digraphs/gap/oper.gi:218 called from
DigraphAddEdges( D, [ [ 1, j * n + 3 ], [ j * n + 3, 1 ] ]
 ); at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:617 called from
BananaTreeCons( IsMutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:627 called from
BananaTreeCons( IsImmutableDigraph, m, n
 ) at /Users/Wilf/gap/pkg/digraphs/gap/examples.gi:636 called from
...  at *stdin*:2
type 'quit;' to quit to outer loop
brk>

@marinaanagno
Copy link
Contributor Author

marinaanagno commented Mar 12, 2021

Thanks for spotting this @wilfwilson working on it just now! I think the error is what would agree with the theory. As it is defined in BananaTree(n, k) precisely one leaf of each of the n copies of S_k is connected to an isolated vertex. If k = 1 we don't have a leaf, we have an isolated vertex for each copy of S_k, so returning an error would seem more reasonable to me, according to the theory, let me know if you have any objection to that and I will proceed differently. :)
Also if 1 didn't give an error, we would just get a symmetric chain graph, and we can already obtain this graph by taking the symmetric closure of ChainDigraph. Thus, I don't feel that adding 1 as an option would be of any use and since the theory implies that a leaf should be connected to the "central" isolated vertex, and an isolated vertex is not a leaf, I will proceed by returning an error if you also agree of course. :)

@wilfwilson
Copy link
Collaborator

Yeah I don't think allowing the second argument to be 1 is useful, so I'm happy to disallow it.

Marina Anagnostopoulou-Merkouri and others added 2 commits March 12, 2021 21:27
doc/examples.xml Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

I'm happy to merge if the tests pass 🙂

@wilfwilson wilfwilson merged commit 00ad997 into digraphs:master Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A label for new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants