Implement `isdigraphical` and fix `isgraphical` #186

InterdisciplinaryPhysicsTeam · 2022-10-30T22:18:03Z

Hello,

We're the developers of MultilayerGraphs.jl.

In this PR we implement the test for graphicality in the directed without loops case (see Fulkerson–Chen–Anstee theorem).

The new function is called isdigraphical, and we tried to implement it as similarly as we could to the already existing isgraphical. Implementation differences are highlighted using comments in the code.

We have already written the docstrings and added tests.

CC: @pitmonticone @ClaudMor

codecov · 2022-10-30T22:27:45Z

Codecov Report

Merging #186 (f2694a6) into master (897e183) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #186      +/-   ##
==========================================
+ Coverage   97.40%   97.43%   +0.03%     
==========================================
  Files         109      113       +4     
  Lines        6470     6554      +84     
==========================================
+ Hits         6302     6386      +84     
  Misses        168      168

aurorarossi

Thank you for the contribution!

src/connectivity.jl

test/connectivity.jl

InterdisciplinaryPhysicsTeam · 2022-12-08T16:14:47Z

Done @aurorarossi,

Thanks for your suggestions.

CC: @ClaudMor @pitmonticone

aurorarossi · 2022-12-08T17:41:58Z

This branch has conflicts because we recently introduced a Julia Formatter. Can you do a merge with Graphs.jl/master? And format the files you changed? I tried to fix it but I don't have permissions to push to your branch. Thanks!

simonschoelly · 2022-12-08T18:13:21Z

src/connectivity.jl

+
+    n = length(indegree_sequence)
+
+    n == length(outdegree_sequence) || return false 


Wouldn't it be more appropriate to throw an exception in this case?

simonschoelly · 2022-12-08T18:13:39Z

src/connectivity.jl

+```
+for each integer r <= n-1
+"""
+function isdigraphical(indegree_sequence::Vector{<:Integer}, outdegree_sequence::Vector{<:Integer} )


Suggested change

function isdigraphical(indegree_sequence::Vector{<:Integer}, outdegree_sequence::Vector{<:Integer} )

function isdigraphical(indegree_sequence::AbstractVector{<:Integer}, outdegree_sequence::AbstractVector{<:Integer} )

Done, but that is in contradiction with the @aurorarossi's request.

Which one?
I suggested changing the signature style of the function in the docstring, but not here

Ops sorry @aurorarossi, we've just noticed that Graphs.jl uses two different conventions for the code and its docstring.

We're going to keep the signature according to @simonschoelly's suggestion then.

simonschoelly · 2022-12-08T18:15:58Z

src/connectivity.jl

+    @inbounds for r = 1:(n - 1)
+
+        indegree_sum += sorted_indegree_sequence[r]
+        outdegree_min_sum = sum([min(sorted_outdegree_sequence[i], r-1) for i in 1:r])


Here we allocate a new Vector in each iteration of the loop. Can we do this differently?

We were not able to use the same strategy adopted in isgraphical. For further details see in the comments.

simonschoelly · 2022-12-08T18:18:36Z

test/connectivity.jl

+    sdg = SimpleDiGraph(10, 90)
+    @test @inferred(isdigraphical(indegree(sdg), outdegree(sdg)))
+    @test !@inferred(isdigraphical([1,1,1], [1,1,0]))
+


Can we also have a few edge cases here? Such as

Empty vectors

Graphs that only have a realization if there is a self-loop.

Graphs where the sum of the in and out degree sequences if the same, but there is still no valid realization.

We've just noticed that isgraphical and isdigraphical don't work properly in the edge cases you mentioned.

For example:

isgraphical([2]) returns true even if it's not possible to realise a simple undirected graph (no self-loops, no multi-edges) with just a vertex with $d(v)=2$.

isdigraphical([1],[1]) returns true even if it's not possible to realise a simple directed graph (no self-loops, no multi-edges) with just a vertex with $d_{in}(v)=d_{out}(v)=1$.

InterdisciplinaryPhysicsTeam · 2022-12-08T18:27:17Z

This branch has conflicts because we recently introduced a Julia Formatter. Can you do a merge with Graphs.jl/master? And format the files you changed? I tried to fix it but I don't have permissions to push to your branch. Thanks!

@aurorarossi, we have just formatted the relevant files.

simonschoelly · 2022-12-15T20:44:39Z

@InterdisciplinaryPhysicsTeam Now there is a lot of noise in this PR because of the formatting changes. Perhaps if you rebase on the latest master branch, they will go away?

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

InterdisciplinaryPhysicsTeam · 2022-12-16T15:09:06Z

Hello @simonschoelly,

We rebased as you asked and updated the isgraphical and isdigraphical functions.

@pitmonticone @ClaudMor

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

InterdisciplinaryPhysicsTeam · 2023-01-12T11:47:03Z

Hello @simonschoelly @aurorarossi @gdalle,

Since we are not maintainers, we need our fork to be on par with Graphs.jl's master branch in order to open new pull requests. Anyway, we cannot do it until this is merged.

May anyone with write permission review and merge this?

simonschoelly · 2023-01-12T15:05:12Z

src/connectivity.jl

+According to Fulkerson-Chen-Anstee theorem, a sequence ``\\{(a_1, b_1), ...,(a_n, b_n)\\}`` (sorted in descending order of a) is graphic iff the sum of vertex degrees is even and  ``\\sum_{i = 1}^{n} a_i = \\sum_{i = 1}^{n} b_i\\}`` and the sequence obeys the property -
+```math
+\\sum_{i=1}^{r} a_i \\leq \\sum_{i=r+1}^n min(r-1,b_i) + \\sum_{i=r+1}^n min(r,b_i)
+```
+for each integer r <= n-1. 


Do you have a source for these conditions? Because the ones on Wikipedia look differently.

I also notices that this algorithm does return incorrect results in some cases:

# would only work with a self-loop, but we don't allow these, right? julia> isdigraphical([1],[1]) true # would require multi-edges julia> isdigraphical([5],[5]) true # but this should also be true if we allow self-loops. julia> isdigraphical([1,0],[1,0]) false

Also, I was wondering if latex heavy docstrings are a good idea? We don't need to change it in this PR, as we always can do it latter, the issue is just that in the REPL it looks like this:

help?> isdigraphical search: isdigraphical isdigraphical(indegree_sequence, outdegree_sequence) Check whether the given indegree sequence and outdegree sequence are digraphical, that is whether they can be the indegree and outdegree sequence of a digraph. Implementation Notes –––––––––––––––––––––– According to Fulkerson-Chen-Anstee theorem, a sequence \{(a_1, b_1), ...,(a_n, b_n)\} (sorted in descending order of a) is graphic iff the sum of vertex degrees is even and \sum_{i = 1}^{n} a_i = \sum_{i = 1}^{n} b_i\} and the sequence obeys the property - \sum_{i=1}^{r} a_i \leq \sum_{i=r+1}^n min(r-1,b_i) + \sum_{i=r+1}^n min(r,b_i) for each integer r <= n-1. See also: isgraphical

We removed the "the sum of vertex degrees is even" part from the docstring of isdigraphical which could be misleading.

The three cases you mentioned should now be fixed and added to tests. Thanks for pointing them out.

We will happily comply with whatever suggestion you give us for formatting docstrings.

simonschoelly · 2023-01-12T15:13:48Z

src/connectivity.jl

+    all(degs .>= 0) ||
+        throw(ArgumentError("The degree sequence must contain non-negative integers only."))


Wouldn't it be more reasonable to just return false here? Of course if that is so, we should probably do the same thing for isdigraphical.

We thought that false was reserved for when "the degree sequence is valid but not graphical", while errors should be thrown if the degree sequence is not even valid.

If it isn't appropriate we will change the behavior.

I leave the decision up too you. One thing that is nice about not throwing an error, that a user does not have to make an extra check for an error, if their input might contain a negative number.

But then we probably should also check, that all degrees are less or equal than n - 1?

Where is would still leave the throws check in, is in the isdigraphical method for inputs of different lengths.

Also note that by throwing an exception in isgraphical we kind of introduce a breaking change - but as this is a very rare edge case, I think that should be ok.

Let me know what you decide.

Thanks for your argumentation.

We agree that returning false allows for simpler usage, and if the user wishes to get an error in specific circumstances, she/he may wrap the function. Thus we now return false when negative degrees are present, or when a degree exceeds n-1.

simonschoelly · 2023-01-12T15:17:36Z

src/connectivity.jl

+    isdigraphical(indegree_sequence, outdegree_sequence)
+
+Check whether the given indegree sequence and outdegree sequence are digraphical, that is whether they can be the indegree and outdegree sequence of a digraph.
+


We should probably mention here that

The digraph cannot have self-loops

indegree_sequence and outdegree_sequence are not independent, i.e. if entry i of indegree_sequence has value a_i and entry i of outdegree_sequence has value b_i then there will be a vertex with indegree a_i and outdegree b_i in the realization. (maybe there is an easier way to express this).

We modified the docstring trying to include your suggestions. Thanks.

simonschoelly · 2023-01-12T15:18:40Z

src/connectivity.jl

+    )
+
+    n = length(indegree_sequence)
+    n == length(outdegree_sequence) || return false


We already check this on line 783.

We removed the redundant check and moved the definition of the variable n to the top of the function.

simonschoelly

Hello @simonschoelly @aurorarossi @gdalle,

Since we are not maintainers, we need our fork to be on par with Graphs.jl's master branch in order to open new pull requests. Anyway, we cannot do it until this is merged.

May anyone with write permission review and merge this?

I am not sure, what you mean, by not being able to open new pull requests?

In any case, I think there are still some bugs here, that need to be fixed before we can merge.

Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>

InterdisciplinaryPhysicsTeam · 2023-01-13T15:49:50Z

Hi @simonschoelly ,

We realized that "not being able to open new pull requests" was due to a mistake of ours, we apologize for that.

We hope that the fixes above comply with your comments.

src/connectivity.jl

simonschoelly · 2023-01-14T07:02:15Z

Some small fixes for the formula in the docstring and some decision to make, if we should throw errors - and then I think we can finally merge this.

Co-authored-by: Simon Schölly <sischoel@gmail.com>

Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>

InterdisciplinaryPhysicsTeam · 2023-01-14T10:06:55Z

We committed your suggestions and accepted your proposal to return false above.

simonschoelly · 2023-01-14T10:51:01Z

Merged, thanks!

aurorarossi requested changes Dec 8, 2022

View reviewed changes

src/connectivity.jl Outdated Show resolved Hide resolved

test/connectivity.jl Outdated Show resolved Hide resolved

test/connectivity.jl Outdated Show resolved Hide resolved

aurorarossi approved these changes Dec 8, 2022

View reviewed changes

simonschoelly reviewed Dec 8, 2022

View reviewed changes

InterdisciplinaryPhysicsTeam changed the title ~~Implement isdigraphical~~ Implement isdigraphical and correct isgraphical Dec 11, 2022

pitmonticone force-pushed the master branch from 6ca4d3d to 897e183 Compare December 16, 2022 14:30

InterdisciplinaryPhysicsTeam and others added 3 commits December 16, 2022 15:47

Make isgraphical more robust and add isdigraphical`

21d9de2

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

Update connectivity.jl

e430a25

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

Format src/connectivity.jl and test/connectivity.jl

9431883

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

Export isdigraphical

a250bed

Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com> Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com>

InterdisciplinaryPhysicsTeam changed the title ~~Implement isdigraphical and correct isgraphical~~ Implement isdigraphical and fix isgraphical Dec 16, 2022

InterdisciplinaryPhysicsTeam mentioned this pull request Jan 12, 2023

Implement Havel-Hakimi and Kleitman-Wang graph realization algorithms #202

Open

simonschoelly reviewed Jan 12, 2023

View reviewed changes

simonschoelly requested changes Jan 12, 2023

View reviewed changes

Fix isdigraphical's main loop, docstring and checks

e8d6258

Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>

simonschoelly reviewed Jan 14, 2023

View reviewed changes

src/connectivity.jl Outdated Show resolved Hide resolved

simonschoelly reviewed Jan 14, 2023

View reviewed changes

src/connectivity.jl Outdated Show resolved Hide resolved

InterdisciplinaryPhysicsTeam and others added 3 commits January 14, 2023 10:58

Update src/connectivity.jl

3c82b52

Co-authored-by: Simon Schölly <sischoel@gmail.com>

Update src/connectivity.jl

76b1bd5

Co-authored-by: Simon Schölly <sischoel@gmail.com>

Update connectivity.jl

f2694a6

Co-Authored-By: Pietro Monticone <38562595+pitmonticone@users.noreply.github.com> Co-Authored-By: Claudio Moroni <43729990+ClaudMor@users.noreply.github.com>

simonschoelly approved these changes Jan 14, 2023

View reviewed changes

simonschoelly merged commit c4360cf into JuliaGraphs:master Jan 14, 2023

pitmonticone mentioned this pull request Feb 27, 2023

Remove isgraphical and isdigraphical using Graphs.jl functions JuliaGraphs/MultilayerGraphs.jl#133

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `isdigraphical` and fix `isgraphical` #186

Implement `isdigraphical` and fix `isgraphical` #186

InterdisciplinaryPhysicsTeam commented Oct 30, 2022 •

edited

codecov bot commented Oct 30, 2022 •

edited

aurorarossi left a comment

InterdisciplinaryPhysicsTeam commented Dec 8, 2022

aurorarossi commented Dec 8, 2022

simonschoelly Dec 8, 2022

InterdisciplinaryPhysicsTeam Dec 11, 2022

simonschoelly Dec 8, 2022

InterdisciplinaryPhysicsTeam Dec 8, 2022

aurorarossi Dec 8, 2022

InterdisciplinaryPhysicsTeam Dec 9, 2022 •

edited

simonschoelly Dec 8, 2022

InterdisciplinaryPhysicsTeam Dec 8, 2022

simonschoelly Dec 8, 2022

InterdisciplinaryPhysicsTeam Dec 9, 2022 •

edited

InterdisciplinaryPhysicsTeam commented Dec 8, 2022

simonschoelly commented Dec 15, 2022

InterdisciplinaryPhysicsTeam commented Dec 16, 2022 •

edited

InterdisciplinaryPhysicsTeam commented Jan 12, 2023

simonschoelly Jan 12, 2023 •

edited

simonschoelly Jan 12, 2023

InterdisciplinaryPhysicsTeam Jan 13, 2023

simonschoelly Jan 12, 2023

InterdisciplinaryPhysicsTeam Jan 13, 2023

simonschoelly Jan 14, 2023

InterdisciplinaryPhysicsTeam Jan 14, 2023

simonschoelly Jan 12, 2023

InterdisciplinaryPhysicsTeam Jan 13, 2023

simonschoelly Jan 12, 2023

InterdisciplinaryPhysicsTeam Jan 13, 2023

simonschoelly left a comment

InterdisciplinaryPhysicsTeam commented Jan 13, 2023

simonschoelly commented Jan 14, 2023

InterdisciplinaryPhysicsTeam commented Jan 14, 2023

simonschoelly commented Jan 14, 2023


		n = length(indegree_sequence)

		n == length(outdegree_sequence) \|\| return false

	function isdigraphical(indegree_sequence::Vector{<:Integer}, outdegree_sequence::Vector{<:Integer} )
	function isdigraphical(indegree_sequence::AbstractVector{<:Integer}, outdegree_sequence::AbstractVector{<:Integer} )

		all(degs .>= 0) \|\|
		throw(ArgumentError("The degree sequence must contain non-negative integers only."))

		isdigraphical(indegree_sequence, outdegree_sequence)

		Check whether the given indegree sequence and outdegree sequence are digraphical, that is whether they can be the indegree and outdegree sequence of a digraph.

Implement isdigraphical and fix isgraphical #186

Implement isdigraphical and fix isgraphical #186

Conversation

InterdisciplinaryPhysicsTeam commented Oct 30, 2022 • edited

codecov bot commented Oct 30, 2022 • edited

Codecov Report

aurorarossi left a comment

Choose a reason for hiding this comment

InterdisciplinaryPhysicsTeam commented Dec 8, 2022

aurorarossi commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InterdisciplinaryPhysicsTeam Dec 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InterdisciplinaryPhysicsTeam Dec 9, 2022 • edited

Choose a reason for hiding this comment

InterdisciplinaryPhysicsTeam commented Dec 8, 2022

simonschoelly commented Dec 15, 2022

InterdisciplinaryPhysicsTeam commented Dec 16, 2022 • edited

InterdisciplinaryPhysicsTeam commented Jan 12, 2023

simonschoelly Jan 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonschoelly left a comment

Choose a reason for hiding this comment

InterdisciplinaryPhysicsTeam commented Jan 13, 2023

simonschoelly commented Jan 14, 2023

InterdisciplinaryPhysicsTeam commented Jan 14, 2023

simonschoelly commented Jan 14, 2023

Implement `isdigraphical` and fix `isgraphical` #186

Implement `isdigraphical` and fix `isgraphical` #186

InterdisciplinaryPhysicsTeam commented Oct 30, 2022 •

edited

codecov bot commented Oct 30, 2022 •

edited

InterdisciplinaryPhysicsTeam Dec 9, 2022 •

edited

InterdisciplinaryPhysicsTeam Dec 9, 2022 •

edited

InterdisciplinaryPhysicsTeam commented Dec 16, 2022 •

edited

simonschoelly Jan 12, 2023 •

edited