WIP: LightGraphs Abstraction #541

sbromberger · 2017-03-10T23:03:16Z

Hi all,

Here's a starting point for abstraction. Comments please.

katz centrality is broken.

sbromberger · 2017-03-10T23:05:09Z

Hierarchy:

AbstractGraph -> AbstractSimpleGraph -> SimpleGraph / SimpleDiGraph

AbstractEdge -> AbstractSimpleEdge -> SimpleEdge

sbromberger · 2017-03-10T23:08:41Z

Also, the docs got a little messed up. Need to fix these.

codecov-io · 2017-03-10T23:09:38Z

Codecov Report

Merging #541 into master will increase coverage by 0.82%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #541      +/-   ##
========================================
+ Coverage   99.17%   100%   +0.82%     
========================================
  Files          57     60       +3     
  Lines        2898   2985      +87     
========================================
+ Hits         2874   2985     +111     
+ Misses         24      0      -24

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9dd2b87...2e1ae20. Read the comment docs.

jpfairbanks · 2017-03-10T23:13:07Z

I like this, my only issue is that there are a lot of methods that are currently mapped to _NI(). Many of these methods could be implemented in terms of each other.

For example fadj(g) can be implemented in terms of out_neighbors(g) so we should pick one and provide a default implementation of the other.

sbromberger · 2017-03-10T23:19:01Z

For example fadj(g) can be implemented in terms of out_neighbors(g) so we should pick one and provide a default implementation of the other.

Can we do the reverse? fadj (and badj, and, for undirected graphs, adj) are really supposed to be accessors for the graph elements themselves (which we've told people explicitly not to use.)

Oh, I see the issue. The problem is that out_neighbors(g,v) assumes an integer for v in SimpleGraphs, and might not be the same for other abstractions.

The methods that equal _NI() in core are core functions that should be defined in abstract graph types.

sbromberger · 2017-03-10T23:37:45Z

My "go-to" check for regression, betweenness_centrality(), shows nothing out of the ordinary:

Current master:

julia> g  = load("graph-5k-50k.jgz")["graph-5000-50000"]
{5000, 50000} directed graph

(precompile deleted)

julia> @time z = betweenness_centrality(g);
 15.461689 seconds (75.54 M allocations: 5.304 GB, 4.10% gc time)

julia> @time z = betweenness_centrality(g);
 15.743490 seconds (75.46 M allocations: 5.301 GB, 4.19% gc time)

This branch:

julia> @time z = betweenness_centrality(g);
 15.433122 seconds (75.46 M allocations: 5.301 GB, 4.36% gc time)

julia> @time z = betweenness_centrality(g);
 15.989293 seconds (75.46 M allocations: 5.301 GB, 4.24% gc time)

…Graphs.jl into sbromberger/benchmarks

sbromberger · 2017-03-14T00:19:35Z

There appears to be a slight regression in betweenness_centrality:

julia> @time z = betweenness_centrality(g);
 17.287090 seconds (75.46 M allocations: 5.301 GB, 4.70% gc time)

edit: no longer - perhaps it's an aberration:

julia> @time z = betweenness_centrality(g);
 15.239393 seconds (75.54 M allocations: 5.304 GB, 4.64% gc time)

sbromberger · 2017-03-14T16:32:28Z

~~Due to mauro3/SimpleTraits.jl#28 we're never going to get 100% code coverage on this PR.~~ We will now that we're 0.6 only.

sbromberger · 2017-03-27T14:29:27Z

Just a heads-up: I spent the weekend updating the docs and making them consistent. There are a few docstrings that need work - I've noted them in comments - but I don't think this will block a merge.

juliohm · 2017-03-27T15:03:45Z

That is an awesome work, I wish could write code with just half your efficiency 👍

I will digest the changes over time by writing more LG tutorials, looking forward to @jpfairbanks review.

sbromberger · 2017-03-27T16:05:15Z

v0.7.4 has been merged into METADATA.jl from master. This is the last 0.5-supported release of LG. I will be finalizing this PR in the next day or so. @jpfairbanks - could you please get ready for final review?

jpfairbanks · 2017-03-27T17:13:32Z

Great Job Seth. This was a lot of work!

How do we want to rebase this?
What about the following? I tried to pick everything that was a substance change and roll up all the testing and fixing commits into one per substance change.

pick 8e4489b simplegraphs abstraction
s 748aa61 f
s 8aee9a8 simplegraphs, take 2
s 2bbafc5 more changes
s 8f1c2af reshuffle
p ced3ee7 fix tests
s 1f6b891 more tests
pick 8922fed abstractions
pic 3099034 more tests
s c1c2e4e tests and fixes
s a8a015e trait fixes and tests - unrolling
s 914c47d persistence and floyd-warshall
s 1f7a269 make(di)graphs, through spanningtrees
s ee54466 moved cliques, testing through connectivity.jl
pick 38bf233 @jpfairbanks first round of review
pick 2c45b45 benchmarks
s 811c509 add edgetype benchmark
pick 5765fa0 Edge is no longer a Pair
pick 6124998 pkgbenchmarks
s 3cb3aa3 remove data files from benchmarks
s 1e22b2c another fix
s 893987d all tests
pick d7e8013 new simpletraits
pick 7b953ee first cut at 0.6 compat
s de342ea squash
pick 293d331 update randgraphs.jl to use Channels over Tasks
s ccb0429 got rid of tasks in randgraphs
s b147c81 graph -> g
pick 5578ab7 Add tutorials to section on docs (#547)
pick dee0528 type -> mutable struct
s 8165d33 more type -> mutable struct, plus OF detection for add_vertex!
p f26c0eb Add tutorials to section on docs (#547)
pick 9396867 WIP: benchmarks (#538)
s 7298222 foo{T}(x::T) -> foo(x::T) where T
pick 3e3b34b test negative cycles
s 1ad6e72 test coverage
pick 97517a4 manual cherry-pick of #551
s bcbe2a9 simplegraph/ -> simplegraphs, optimization for is_connected, some type simplifications
s f06aa93 re-add b-f tests
s c61011f Inferred (#554)
s e6ce33d redo graphmatrices tests
s 2a937bd cherry pick bellman-ford test
s d03a0e5 add more bellman-ford negative cycles (#551)
s 9dd2b87 short circuit is_connected (#552)
pick 9df52ad linalg test fix
pick 91df2ab loosen type restrictions in randgraphs functions
pick ce00535 readall -> readstring, and comment rationalization in randgraphs
p 3d770d5 Fixes #555: graphmatrices convert incorrect on CA (#560)
s ab176b8 fixes #564
s 49b160c one more test
p dfe72cb removed nv() and vertices() (#565)
p 4345a3e simpleedge tests
s 23e7123 test coverage
s 0e21aa3 short circuit B-F negative cycles, plus tests
s bcbd873 more test coverage
s e733cfe more test coverage

sbromberger · 2017-03-27T18:34:29Z

@jpfairbanks I like your approach outlined above. I'm really sorry this is such a mess of commits, but I wanted to break them out into smaller units of change so that they would be easy to bisect/diagnose.

I have one more major commit to make (merge the new docs); it has some additional code bugfixes, found when the docs didn't match the output, and some cleanup of variable names. It shouldn't cause any major impact though.

Can you rebase? I don't trust my git skills to do it myself, and my original plan (don't laugh) was just to "squash and merge".

* docs, plus some various fixes (see blocking_flow). * nodes -> vertices, node -> vertex, and more doc consistency

jpfairbanks · 2017-03-29T15:05:07Z

I gave rebasing this thing a try, but there are a lot of conflicts and the resolution choices you make compound, I got about halfway through. If someone else wants to try and rebase it, go ahead.

I think we can squash and merge this behemoth into one big commit. Most of the internal state will not really be useful anyway.

LGTM soon.

sbromberger · 2017-03-29T15:24:46Z

@jpfairbanks thanks for the attempt. I figured it just became too big to manage properly.

Is there any reason not to merge into master at this point? There's one other PR depending on this one.

sbromberger · 2017-03-29T15:25:53Z

I suppose we should run some benchmarks.

jpfairbanks · 2017-03-29T16:08:47Z

I am fine with merging and fixing any regressions in a separate PR

jpfairbanks

Squash this massive PR and let's all agree to never have a PR with 7K+ lines of diff again.

sbromberger · 2017-03-29T16:41:49Z

let's all agree to never have a PR with 7K+ lines of diff again.

hah! how else could I have done this? (Genuine question.)

tkelman · 2017-04-09T12:31:54Z

src/flow/ext_multiroute_flow.jl

-  )
-  return a[1] ≈ b[1] && a[2] ≈ b[2]
-end
+≈(a::Tuple{T, T}, b::Tuple{T, T}) where T <: AbstractFloat =


this is type piracy and changes global behavior - could you do this instead with a different operator name, or via all(a .≈ b) to not extend arithmetic operators to tuples?

The global behavior is undefined: there is no isapprox method for tuples, so nobody should be using it.

We're only using it in one place, so I can rename it, but it's (at least for me, and likely for the author) a really intuitive extension of isapprox.

sure, people have tried adding arithmetic to tuples in a bunch of places, but it has surprising side effects because the language uses tuples for argument lists and return values

Good point. I'm renaming now, but I've also filed JuliaLang/julia#21336 for discussion.

Thanks. Do I close the METADATA PR and re-release v0.8.0 with this new code?

…0540312

Merging due to exigencies.

tkelman · 2017-04-10T02:21:23Z

src/interface.jl

+
+Return a zero-vertex, zero-edge version of the same type of graph as `g`.
+"""
+zero(x...) = _NI("zero")


should this be zero(g::AbstractGraph) ? defining zero-arg zero() and vararg zero for Any inputs seems odd

sbromberger and others added 4 commits March 8, 2017 21:26

benchmarks

2c45b45

add edgetype benchmark

811c509

katz centrality is broken.

Merge branch 'master' into sbromberger/benchmarks

c704c68

simplegraphs abstraction

8e4489b

sbromberger mentioned this pull request Mar 10, 2017

Scaffolding discussion #527

Closed

sbromberger mentioned this pull request Mar 10, 2017

WIP: benchmarks #538

Merged

6 tasks

sbromberger added 8 commits March 12, 2017 09:21

Edge is no longer a Pair

5765fa0

Merge branch 'sbromberger/benchmarks' of github.com:JuliaGraphs/Light…

bfb0751

…Graphs.jl into sbromberger/benchmarks

pkgbenchmarks

6124998

Merge branch 'master' into sbromberger/benchmarks

4944904

Merge branch 'master' into sbromberger/simplegraphs

6afd3dc

f

748aa61

remove data files from benchmarks

3cb3aa3

simplegraphs, take 2

8aee9a8

sbromberger added 4 commits March 13, 2017 21:57

more changes

2bbafc5

reshuffle

8f1c2af

fix tests

ced3ee7

more tests

1f6b891

sbromberger mentioned this pull request Mar 27, 2017

0.8 Milestone #561

Closed

8 tasks

sbromberger and others added 2 commits March 27, 2017 13:13

Docs (#567)

727fe47

* docs, plus some various fixes (see blocking_flow). * nodes -> vertices, node -> vertex, and more doc consistency

doc fixes

e20781c

sbromberger modified the milestones: v0.8, 1.0 Mar 28, 2017

sbromberger mentioned this pull request Mar 28, 2017

connected_components dispatch too broad? #368

Closed

sbromberger added 3 commits March 27, 2017 20:27

1.0 -> 0.8

d8d12ce

docfix and benchmarks

969b6b2

doc fixes

2e1ae20

iamed2 mentioned this pull request Mar 28, 2017

Upper bounds on supported Julia versions #570

Closed

jpfairbanks approved these changes Mar 29, 2017

View reviewed changes

sbromberger merged commit c93b73a into master Mar 29, 2017

sbromberger deleted the sbromberger/simplegraphs branch March 29, 2017 17:32

jpfairbanks mentioned this pull request Mar 29, 2017

Large PRs #576

Closed

tkelman reviewed Apr 9, 2017

View reviewed changes

sbromberger added a commit that referenced this pull request Apr 9, 2017

https://github.com/JuliaGraphs/LightGraphs.jl/pull/541#discussion_r11…

2d997e1

…0540312

sbromberger mentioned this pull request Apr 9, 2017

fix for v0.8.0 and type piracy #581

Merged

sbromberger added a commit that referenced this pull request Apr 9, 2017

#541 (comment) (#581)

dd561b3

Merging due to exigencies.

tkelman reviewed Apr 10, 2017

View reviewed changes

sbromberger mentioned this pull request Mar 8, 2018

remove long-term deprecation warnings #863

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: LightGraphs Abstraction #541

WIP: LightGraphs Abstraction #541

sbromberger commented Mar 10, 2017

sbromberger commented Mar 10, 2017

sbromberger commented Mar 10, 2017

codecov-io commented Mar 10, 2017 •

edited by codecov bot

Loading

jpfairbanks commented Mar 10, 2017

sbromberger commented Mar 10, 2017 •

edited

Loading

sbromberger commented Mar 10, 2017

sbromberger commented Mar 14, 2017 •

edited

Loading

sbromberger commented Mar 14, 2017 •

edited

Loading

sbromberger commented Mar 27, 2017

juliohm commented Mar 27, 2017

sbromberger commented Mar 27, 2017

jpfairbanks commented Mar 27, 2017

sbromberger commented Mar 27, 2017

jpfairbanks commented Mar 29, 2017

sbromberger commented Mar 29, 2017

sbromberger commented Mar 29, 2017

jpfairbanks commented Mar 29, 2017

jpfairbanks left a comment

sbromberger commented Mar 29, 2017

tkelman Apr 9, 2017 •

edited

Loading

sbromberger Apr 9, 2017

sbromberger Apr 9, 2017

tkelman Apr 9, 2017

sbromberger Apr 9, 2017

tkelman Apr 9, 2017

sbromberger Apr 9, 2017

tkelman Apr 10, 2017 •

edited

Loading

WIP: LightGraphs Abstraction #541

WIP: LightGraphs Abstraction #541

Conversation

sbromberger commented Mar 10, 2017

sbromberger commented Mar 10, 2017

sbromberger commented Mar 10, 2017

codecov-io commented Mar 10, 2017 • edited by codecov bot Loading

Codecov Report

jpfairbanks commented Mar 10, 2017

sbromberger commented Mar 10, 2017 • edited Loading

sbromberger commented Mar 10, 2017

sbromberger commented Mar 14, 2017 • edited Loading

sbromberger commented Mar 14, 2017 • edited Loading

sbromberger commented Mar 27, 2017

juliohm commented Mar 27, 2017

sbromberger commented Mar 27, 2017

jpfairbanks commented Mar 27, 2017

sbromberger commented Mar 27, 2017

jpfairbanks commented Mar 29, 2017

sbromberger commented Mar 29, 2017

sbromberger commented Mar 29, 2017

jpfairbanks commented Mar 29, 2017

jpfairbanks left a comment

Choose a reason for hiding this comment

sbromberger commented Mar 29, 2017

tkelman Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

sbromberger Apr 9, 2017

Choose a reason for hiding this comment

sbromberger Apr 9, 2017

Choose a reason for hiding this comment

tkelman Apr 9, 2017

Choose a reason for hiding this comment

sbromberger Apr 9, 2017

Choose a reason for hiding this comment

tkelman Apr 9, 2017

Choose a reason for hiding this comment

sbromberger Apr 9, 2017

Choose a reason for hiding this comment

tkelman Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Mar 10, 2017 •

edited by codecov bot

Loading

sbromberger commented Mar 10, 2017 •

edited

Loading

sbromberger commented Mar 14, 2017 •

edited

Loading

sbromberger commented Mar 14, 2017 •

edited

Loading

tkelman Apr 9, 2017 •

edited

Loading

tkelman Apr 10, 2017 •

edited

Loading