-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add BananaTree #411
Conversation
@alex-konovalov thank you so much for your comments! Fixed them just now! :) |
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
Thank you @wilfwilson! I'm really sorry, I have no idea how I managed to do this, I'll try and fix it now! :) |
@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 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 :) |
There was a problem hiding this 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!
Happy to help whenever, just send me a message and we can do some screen sharing if you think that would help. |
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>
There was a problem hiding this 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
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"/>. |
There was a problem hiding this comment.
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
.
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 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 :) |
There was a problem hiding this 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>
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. :) |
Yeah I don't think allowing the second argument to be |
There was a problem hiding this 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 🙂
No description provided.