sbromberger / LightGraphs.jl

An optimized graphs package for the Julia programming language
Other
671 stars 185 forks source link

compose() shadows Compose.compose #35

Closed jiahao closed 9 years ago

jiahao commented 9 years ago

The compose function shadows Compose.jl's primary function, compose, which makes it very difficult to work with together, e.g. when drawing custom graph layouts building upon GraphLayout.jl.

using Compose
using LightGraphs
compose(context(), rectangle(0, 0, 1, 1))
#Error:`compose` has no method matching compose(::Context, ::Form{RectanglePrimitive{P<:Point{XM<:Measure{S,T},YM<:Measure{S,T}},M1<:Measure{S,T},M2<:Measure{S,T}}})
using Compose
compose(context(), rectangle(0, 0, 1, 1))
using LightGraphs
#Warning: using LightGraphs.compose in module Main conflicts with an existing identifier.

Can we do something about this? Maybe rename compose to something else?

sbromberger commented 9 years ago

I don't have an issue with renaming it (to, say, merge) - the name comes from the NetworkX function of the same name.

A bigger question, though: what's "standard practice" when two function names collide in Julia?

sbromberger commented 9 years ago

Closing per #36. @jiahao - I have some other things that I'd like to get into the next tagged version so please go ahead and use Pkg.checkout() for now, if that's ok. Thanks!

jpfairbanks commented 9 years ago

It might be good to have what is no called merge be union and have what is now called union be blkdiag to match the sparse matrix operation.

Then we have the properties:

union(g, h) = graph(union(V(g), V(h)), union (E(g), E(h))).

And

adjmat(blkdiag(g, h)) = blkdiag(adjmat(g), adjmat(h)) .

The symmetry is pleasing and makes it easy to understand.

Not a crisis but better to get the names right now when the user base is small than in the future when it will disrupt many users.

sbromberger commented 9 years ago

I took the names directly from the NetworkX functions that perform the same activity. In general I'd like to keep in parallel with that package since it's what I'm most familiar with :) Renaming compose to merge was a one-off to avoid issues with another package.

jiahao commented 9 years ago

There's really no way to deal with name collisions of this form in Julia. The best one can do is to test if the generic function is already defined, and if so, import it before defining the new method, and hope that there are no ambiguity warnings. If you do all this, then if you load Compose then LightGraphs, then should work. It will still fail if you load LightGraphs then Compose because Compose needs to do the same thing.

@sbromberger I can see the utility of paralleling the NetworkX names; however @jpfairbanks has a point about mapping the functionality onto new methods of existing functions in base Julia. It's worth thinking about.

jpfairbanks commented 9 years ago

It speaks to a broader point in Julia because we went to be easy to migrate Matlab R and Python users to Julia but we also need to make sure we are internally consistent. On Apr 4, 2015 10:54 AM, "Jiahao Chen" notifications@github.com wrote:

There's really no way to deal with name collisions of this form in Julia. The best one can do is to test if the generic function is already defined, and if so, import it before defining the new method, and hope that there are no ambiguity warnings. If you do all this, then if you load Compose then LightGraphs, then should work. It will still fail if you load LightGraphs then Compose because Compose needs to do the same thing.

@sbromberger https://github.com/sbromberger I can see the utility of paralleling the NetworkX names; however @jpfairbanks https://github.com/jpfairbanks has a point about mapping the functionality onto new methods of existing functions in base Julia. It's worth thinking about.

— Reply to this email directly or view it on GitHub https://github.com/JuliaGraphs/LightGraphs.jl/issues/35#issuecomment-89590534 .

sbromberger commented 9 years ago

Ok, I'm open to renaming. Sorry for pushing back - I guess I can be a bit of a grump when it comes to changing something I know already (been using NX for years). Can someone make a PR?

Also, what's adjmat?

jpfairbanks commented 9 years ago

My shorthand for adjacency matrix. On Apr 4, 2015 11:13 AM, "Seth Bromberger" notifications@github.com wrote:

Ok, I'm open to renaming. Sorry for pushing back - I guess I can be a bit of a grump when it comes to changing something I know already (been using NX for years). Can someone make a PR?

Also, what's adjmat?

— Reply to this email directly or view it on GitHub https://github.com/JuliaGraphs/LightGraphs.jl/issues/35#issuecomment-89593347 .

sbromberger commented 9 years ago

of course - sorry - it's 0819 here and I haven't had caffeine.

I get it now, and I like the elegance. Please disregard my earlier objections.

sbromberger commented 9 years ago

Reopening to track second rename effort.

sbromberger commented 9 years ago

Closing per #43.