Repository navigation
Add DigraphByEdges variant for list of arbitrary vertex labels and list of edges - #957
William-Kenyon wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (90.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #957 +/- ##
==========================================
- Coverage 97.47% 97.45% -0.03%
==========================================
Files 51 53 +2
Lines 21301 21446 +145
Branches 639 639
==========================================
+ Hits 20763 20900 +137
- Misses 473 481 +8
Partials 65 65 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
William-Kenyon
added
the
VIP
james-d-mitchell
left a comment
james-d-mitchell
left a comment
There was a problem hiding this comment.
Thanks for the PR @William-Kenyon. I think what you've implemented should be a method for DigraphByEdges and not an overload of Digraph though. Can you please update it so that it is this, and address the other comments too? Thanks!
| fi; | ||
| if not (IsSubset(vertex_labels, Union(edges))) then |
There was a problem hiding this comment.
| fi; | |
| if not (IsSubset(vertex_labels, Union(edges))) then | |
| elif not IsSubset(vertex_labels, Union(edges)) then |
save one line, and the brackets are superfluous, so please remove.
| ErrorNoReturn("<vertex_labels> contains duplicate labels,"); | ||
| fi; | ||
| if not (IsSubset(vertex_labels, Union(edges))) then | ||
| ErrorNoReturn("<edges> references a label that isn't in <vertex_labels>,"); |
There was a problem hiding this comment.
It's be useful if this line could include some details of which label is bad (position, and value), and the value of vertex_labels maybe too?
William-Kenyon
changed the title
Digraph constructor for list of vertex labels and list of edgesDigraphByEdges variant for list of arbitrary vertex labels and list of edges
| duplicates := List(Filtered(label_counts, x -> x[2] > 1), x -> x[1]); | ||
| if not IsEmpty(duplicates) then | ||
| ErrorNoReturn("<vertex_labels> contains duplicate labels: ", | ||
| duplicates, ","); |
There was a problem hiding this comment.
| duplicates, ","); | |
| duplicates); |
There was a problem hiding this comment.
To explain at some point in the distant past GAP errors were hard to parse, and we inserted a comma at the end of every error to delineate our comments from the general error handling in GAP. I don't think this is any longer necessary, so we don't need the commas any more. I will try to open a PR just now to remove the remaining instances.
james-d-mitchell
left a comment
james-d-mitchell
left a comment
There was a problem hiding this comment.
This looks great. There are a few minor things to update in the error messages. This might be a separate issue but what if I want to do: DigraphByEdges(IsMutableDigraph, ...) maybe this doesn't exist already, if not, then we should add an issue to implement this (so not part of the current PR).
|
|
||
| label_counts := Collected(vertex_labels); | ||
| duplicates := List(Filtered(label_counts, x -> x[2] > 1), x -> x[1]); | ||
| if not IsEmpty(duplicates) then |
There was a problem hiding this comment.
I'd be tempted to just call IsDuplicateFreeList here (i.e. if not IsDuplicateFreeList(vertex_labels) then) and only then do the Collected line and duplicates if this fails. This way we'll avoid the overhead, unless something goes wrong, in which case we don't care about the overhead anymore.
| duplicates := List(Filtered(label_counts, x -> x[2] > 1), x -> x[1]); | ||
| if not IsEmpty(duplicates) then | ||
| ErrorNoReturn("<vertex_labels> contains duplicate labels: ", | ||
| duplicates, ","); |
There was a problem hiding this comment.
To explain at some point in the distant past GAP errors were hard to parse, and we inserted a comma at the end of every error to delineate our comments from the general error handling in GAP. I don't think this is any longer necessary, so we don't need the commas any more. I will try to open a PR just now to remove the remaining instances.
| badpos := PositionProperty(edges, e -> not IsList(e) or Length(e) <> 2); | ||
| if badpos <> fail then | ||
| ErrorNoReturn("invalid edge ", edges[badpos], " in position ", badpos, | ||
| " in <edges>; edges must be pairs of elements,"); |
There was a problem hiding this comment.
| " in <edges>; edges must be pairs of elements,"); | |
| " in <edges>; edges must be lists of length 2,"); |
| e -> not (e[1] in labels and e[2] in labels)); | ||
| if badpos <> fail then | ||
| ErrorNoReturn("invalid edge ", edges[badpos], " in <edges> at position ", | ||
| badpos, "; edges must reference vertices from ", |
There was a problem hiding this comment.
| badpos, "; edges must reference vertices from ", | |
| badpos, "; edges must only contain items from <vertex_labels> (= ", |
| fi; | ||
|
|
||
| nrverts := Length(vertex_labels); | ||
| D := DigraphByEdges(List(edges, |
There was a problem hiding this comment.
Probably this should be the method for DigraphByEdgesNC(vertex_labels, edges)
Fixes #778, for adding a way of constructing a
Digraphwith two lists:vertex_labels, andedges, where vertex labels can be more than just integers. This is basically the same as the existing 3-argument constructor (vertices,source,range), exceptsourceandrangeare zipped together.Based on @wilfwilson's code snippet.