Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

0.8 Milestone #561

Closed
7 of 8 tasks
jpfairbanks opened this issue Mar 23, 2017 · 14 comments
Closed
7 of 8 tasks

0.8 Milestone #561

jpfairbanks opened this issue Mar 23, 2017 · 14 comments
Labels
discussion requires in-depth discussion - participation encouraged
Milestone

Comments

@jpfairbanks
Copy link
Contributor

jpfairbanks commented Mar 23, 2017

We should plan to hit v0.8 soon.
What are the things we need to do to get there.

@jpfairbanks jpfairbanks added this to the 1.0 milestone Mar 23, 2017
@sbromberger
Copy link
Owner

Do we want to deprecate nv? That's a quick fix. I can do that tonight.

@sbromberger
Copy link
Owner

I'd like to move persistence out to LGE if we can, but it can wait until 1.1.

@sbromberger
Copy link
Owner

Ref JuliaLang/METADATA.jl#8454

@sbromberger sbromberger added the discussion requires in-depth discussion - participation encouraged label Mar 23, 2017
@jpfairbanks
Copy link
Contributor Author

According to the Semantic Versioning definition removing persistence would be a breaking change that would require a major version number bump, unless you think that telling people to import LGE to use persistence is not breaking. All the functions will stay the same right?

The problem is that we put stuff in LGE that needs a lot of binary dependencies people will need that to have persistence working.

@sbromberger
Copy link
Owner

sbromberger commented Mar 27, 2017

I think in 1.1 we can @deprecate the functions we move into LGE. I understand this might be a semver violation but I'd really rather not hold up 1.0 for the persistence changes, especially since we've got such a huge change in the pipeline already with #541.

On the other hand, LGE was originally supposed to be a stopgap measure for things that were too heavy-weight for LG. My long-term goal for that package was to break each bit of functionality out into its own separate package in the same organization (IIRC, this is why I registered JuliaGraphs).

Perhaps we can start that process by making "LightGraphsPersistence.jl" the first completely separate (optional) package. Thoughts?

@jpfairbanks
Copy link
Contributor Author

jpfairbanks commented Mar 27, 2017 via email

@sbromberger
Copy link
Owner

GraphPersistence.jl? Where it works for any graph type?

This is an important bikeshed, actually. LightGraphs used to refer to what are now called SimpleGraphs (I got tired of waiting for https://github.com/scheinerman/SimpleGraphs.jl to be registered, so I took the name for use as a type). It now refers to just the package, and not necessarily the type of graph.

When you say "any graph type" do you mean other graph packages? (How would we do that?)

@sbromberger sbromberger changed the title 1.0 Milestone 0.8 Milestone Mar 28, 2017
@sbromberger sbromberger modified the milestones: 1.0, v0.8 Mar 28, 2017
@jpfairbanks
Copy link
Contributor Author

I think that packages that are data structure independent, that is provide functionality for any subtype of AbstractGraph, do not need LightGraph in the name. So GraphVisualization.jl, and GraphPersistence.jl make a lot of sense to me.

These packages would look like

module GraphPersistence
using LightGraphs

function save(path::String, g::AbstractGraph)
    open(path, "w") do
        # write graph to file
    end
end

Then when you go to use them you do

using LightGraphs.SimpleGraphs
using GraphPersistence

g = SimpleGraph()
# insert some edges
# yadayada
save("graph.gml", g)

If you have a package that defines its own subtypes of AbstractGraph you would write:

using DatabaseGraphs
using GraphPersistence

g = DatabaseGraph("postgress://user:[email protected]/dbname",
                                  "SELECT src,dst FROM edgetable")
save("graph.gml", g)

where DatabaseGraph <: AbstractGraph and the constructor takes a database connection string and a query and makes implements a graph interface on top of that.

These data structure independent packages are using only the AbstractGraph interface from LightGraphs.

@sbromberger
Copy link
Owner

@jpfairbanks saving works well since the arguments can be abstract, but what do we do about loading?

@jpfairbanks
Copy link
Contributor Author

Can we pass an empty graph of the given type and have the loading routines fill it in?

These packages would look like

module GraphPersistence
using LightGraphs

function load!(path::String, g::AbstractGraph)
    open(path, "r") do file
        for edge in edges(file)
              add_edge!(g, edge)
        end
    end
    return g
end

Then when you go to use them you do

using LightGraphs.SimpleGraphs
using GraphPersistence

g = SimpleGraph()
load!("graph.gml", g)
print(g)

If you are writing a package that defines its own subtypes of AbstractGraph you would write:

module DatabaseGraphs
using GraphPersistence

function add_edge!(g::DatabaseGraph, edge)
    execute(g.connection, "insert ?,? into edgetable", src(edge), dst(edge))
end

g = DatabaseGraph("postgress://user:[email protected]/dbname")
load!("graph.gml", g)
print(g)

@sbromberger
Copy link
Owner

That means we can't use FileIO, which I really want to do.

The whole persistence functionality needs to be reworked. My preference is to get it out of core (per #557) and then figure out a good way of doing things.

(My other preference is to keep the LightGraphs persistence functionality inside core since 1) it only depends on GZip and 2) it gives at least one way to load/save graphs without having to depend on other packages.)

@jpfairbanks
Copy link
Contributor Author

Based on the description here: https://github.com/JuliaIO/FileIO.jl#implementing-loaderssavers
Can't we define a keyword argument and have the user provide an empty graph using that keyword argument?

I tested the ideas here:

julia> function load{S}(a::String, b::S=0)
       c=S(one(b)); d=c-b; return a,b,c,d
       end
load (generic function with 2 methods)

julia> load("a", 3//2)
("a", 3//2, 1//1, -1//2)

julia> @code_warntype load("a", 3//2)
Variables:
  #self#::#load
  a::String
  b::Rational{Int64}
  c::Rational{Int64}
  d::Rational{Int64}

Body:
  begin
      SSAValue(0) = $(Expr(:invoke, MethodInstance for Rational{Int64}(::Int64, ::Int64), :($(QuoteNode(Rational{Int64}))), 1, 1))
      c::Rational{Int64} = (Core.typeassert)($(Expr(:invoke, MethodInstance for Rational{Int64}(::Int64, ::Int64), :($(QuoteNode(Rational{Int64}))), :((Core.getfield)(SSAValue(0), :num)::Int64), :((Core.getfield)(SSAValue(0), :den)::Int64))), Rational{Int64})::Rational{Int64} # line 2:
      d::Rational{Int64} = $(Expr(:invoke, MethodInstance for -(::Rational{Int64}, ::Rational{Int64}), :(Main.-), :(c), :(b))) # line 2:
      return (Core.tuple)(a::String, b::Rational{Int64}, c::Rational{Int64}, d::Rational{Int64})::Tuple{String,Rational{Int64},Rational{Int64},Rational{Int64}}
  end::Tuple{String,Rational{Int64},Rational{Int64},Rational{Int64}}

@sbromberger
Copy link
Owner

Yup, I guess that would do it, but man. Having load! as a mutating function really doesn't sit well with me. In any case, we should move this discussion to #569.

@sbromberger
Copy link
Owner

Closing now that v0.8.0 has been tagged and released.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion requires in-depth discussion - participation encouraged
Projects
None yet
Development

No branches or pull requests

2 participants