Skip to content

Conversation

RheyaM
Copy link

@RheyaM RheyaM commented Oct 1, 2025

Created SwapDigraphs to swap the contents of two digraphs, and added DigraphsRemoveAllEdges for removing all edges from a digraph

mtorpey
mtorpey previously requested changes Oct 1, 2025
Copy link
Collaborator

@mtorpey mtorpey left a comment

Choose a reason for hiding this comment

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

Great work, especially for your first pull request. I've suggested a load of changes (we always do this) so it'd be great if you could make those changes and we can get this merged. Sorry for the major changes I'm requesting to the core algorithm!

@wilfwilson wilfwilson changed the title Added SwapDigraphs and DigraphsRemoveAllEdges functions Add SwapDigraphs and DigraphsRemoveAllEdges functions Oct 1, 2025
Copy link
Member

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

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

Excellent start, but this Pr will need some changes before it can be merged, let's discuss tomorrow!

@mtorpey mtorpey dismissed their stale review October 8, 2025 11:16

Mostly fixed, with remaining problems covered by @james-d-mitchell's review.

Copy link
Collaborator

@mtorpey mtorpey left a comment

Choose a reason for hiding this comment

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

Very close to complete now. Just a couple more comments.

<Attr Name="DigraphRemoveAllEdgesAttr" Arg="digraph"/>
<Returns>An empty digraph.</Returns>
<Description>
This operation returns a digraph constructed from <A>digraph</A>, which is mutable,
Copy link
Collaborator

Choose a reason for hiding this comment

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

"which is mutable" is out of date – it might not be mutable, as you've explained below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A sentence including the word "if" would be better. :)

end);

InstallMethod(SwapDigraphs,
"for a mutable digraph and mutable digraph",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better would be "for two mutable digraphs".

<mutable digraph with 3 vertices, 3 edges>
gap> DigraphRemoveAllEdges(D3);
<mutable empty digraph with 3 vertices>
true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure where "true" came from. Is this included by accident? I'm surprised it passes the tests!

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.

4 participants