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

Make behaviour of DigraphRemoveEdge{s} consistent when removing no edges #340

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Peter-Ing
Copy link

@Peter-Ing Peter-Ing commented Nov 11, 2020

Added an additional method to preserve properties in the case of empty removals.

This addresses issue #260. (Note from Wilf: currently only partially.)

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.

Please can you also add a test for this behaviour, which verifies that the digraph return by DigraphRemoveEdges, when given an immutable digraph D and an empty list, is indeed the identical object to D.

Please you can you also fixed the spelling mistakes in the commit message, and indeed reformulate the commit message, so that its first line (if possible) consists of 50 or fewer characters?

gap/oper.gi Outdated Show resolved Hide resolved
@wilfwilson
Copy link
Collaborator

Oh wait, what is the relationship between this PR and #338?

@james-d-mitchell james-d-mitchell changed the title modified: gap/oper.gi added addiontal method to preserve propertie… modified: gap/oper.gi added additional method to preserve propertie… Nov 16, 2020
@Peter-Ing
Copy link
Author

Oh wait, what is the relationship between this PR and #338?

this request should have been an update to #338 but as it hasn't updated it and comments have been left here I have closed #338.

@james-d-mitchell
Copy link
Member

@Peter-Ing I agree with @wilfwilson. I think the intended changes would be the following:

if digraph is immutable:

  • DigraphRemoveEdges(digraph, [ ]) returns digraph
  • DigraphRemoveEdge(digraph, [ ]) gives the current error (the argument is not valid)
  • DigraphRemoveEdges(digraph, [[src, ran]]) returns an immutable copy of digraph only when [src, ran] is valid. If [src, ran] is not valid, then the function could either give an error, or just return digraph (i.e. not a copy)
  • DigraphRemoveEdge(digraph, [src, ran]) returns an immutable copy of digraph only when [src, ran] is valid. If [src, ran] is not valid, then the function could either give an error, or just return digraph (i.e. not a copy)

I'm not sure which is preferable: giving an error or just returning digraph and doing nothing. The latter would be more consistent with the current behaviour, but the former would probably be more helpful. Any thoughts?

@james-d-mitchell james-d-mitchell changed the title modified: gap/oper.gi added additional method to preserve propertie… Resolve Issue #260 Nov 18, 2020
@james-d-mitchell james-d-mitchell added the enhancement A label for PRs that provide enhancements. label Nov 18, 2020
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.

Thanks @Peter-Ing, I think this PR should be reworked to reflect the comments I made, feel free to leave it open, and just rework your new changes on top of this.

@wilfwilson
Copy link
Collaborator

I tend to prefer the former kind of behaviour, I think it is what most people (or at least I) would expect. The latter kind of behaviour seems like it's done for optimisation, letting you reduce code (not having a separate existence check first, in some cases) and save time (by not twice checking whether an edge exists ). Unless users ask for that kind of behaviour, I don't think we should proactively offer it.

@codecov

This comment has been minimized.

@wilfwilson wilfwilson changed the title Resolve Issue #260 Fix issue #260: Consistent behaviour of DigraphRemoveEdge{s} when removing no edges Mar 3, 2021
@wilfwilson wilfwilson changed the title Fix issue #260: Consistent behaviour of DigraphRemoveEdge{s} when removing no edges Make behaviour of DigraphRemoveEdge{s} consistent when removing no edges Mar 3, 2021
@wilfwilson wilfwilson changed the title Make behaviour of DigraphRemoveEdge{s} consistent when removing no edges Make behaviour of DigraphRemoveEdge{s} consistent when removing no edges Mar 26, 2021
@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Stale pull request message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A label for PRs that provide enhancements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants