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

RFC: (licensing issue?) Centrality measures #129

Closed
wants to merge 2 commits into from
Closed

RFC: (licensing issue?) Centrality measures #129

wants to merge 2 commits into from

Conversation

sbromberger
Copy link
Contributor

Hi,

I needed some centrality measures, but noticed that none were available - so I decided to start one. First up is degree_centrality, since it's the easiest. My hope is to eventually get betweenness centrality in there as well. I'll be using the networkx package as inspiration.

@garborg
Copy link
Contributor

garborg commented Dec 15, 2014

Cool. networkx looks like it's BSD licensed -- I forget whether 2-clause or
3-clause is the one that's compatible with the MIT license (depending on
the specific BSD license, if you've looked at the code, you may not be able
to contribute this, and other contributors may need to avoid reviewing) --
anyone know?

On Mon, Dec 15, 2014 at 9:50 AM, Seth Bromberger [email protected]
wrote:

Hi,

I needed some centrality measures, but noticed that none were available -
so I decided to start one. First up is degree_centrality, since it's the
easiest. My hope is to eventually get betweenness centrality in there as

well. I'll be using the networkx package as inspiration.

You can merge this Pull Request by running

git pull https://github.com/sbromberger/Graphs.jl centrality

Or view, comment on, or merge it at:

#129
Commit Summary

  • centrality measures - initial commit
  • centrality measures - initial commit
  • Merge branch 'centrality' of github.com:sbromberger/Graphs.jl into
    centrality

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#129.

@sbromberger
Copy link
Contributor Author

License is http://networkx.github.io/documentation/development/reference/legal.html - I'm not incorporating the source, or the binary - just the algorithms referenced in the documentation. IANAL but I can't see this as a problem. If it helps, I can contact Dr Hagberg and seek his permission....

@garborg
Copy link
Contributor

garborg commented Dec 15, 2014

IANAL, but that sounds right -- just figured it was better to see whether licensing clarification was needed before anyone reviewed, just in case. Thanks (and disregard)!

@sbromberger
Copy link
Contributor Author

It's a good question. For this particular measure, I did not rely on networkx at all (except to run the graphs in parallel there to ensure I got the calculations right). Degree centrality is fairly straightforward. When we get to other measurements, though, I'll probably need to mimic the networkx stuff since it's stretching the bounds of my graph knowledge :)

Bottom line: networkx was not used in the creation of the centrality code to this point. It's safe to review under any interpretation of any license.

@sbromberger sbromberger changed the title Centrality measurea Centrality measures Dec 15, 2014
@johnmyleswhite
Copy link
Contributor

As long as you never read any code, you're totally fine. Also, I think you're fine using BSD code inside of this project, you just need to annotate those files as BSD. BSD isn't viral, so it just means that the derived file is BSD, without making the project BSD.

@sbromberger
Copy link
Contributor Author

Oh, wait. I fully plan on reading the code. I haven't done it to date*, but I plan on doing it for the hairier measurements. Should I just put the license in a comment block at the top? Would that be acceptable to this project?

*actually, that's not quite true as I've been a contributor to the networkx project in the past. I just haven't done it for THIS particular effort so far.

@sbromberger sbromberger changed the title Centrality measures RFC: (licensing issue?) Centrality measures Dec 16, 2014
@sbromberger
Copy link
Contributor Author

I added the BSD license block from the NetworkX page, and will put all functions derived from their package below. Will that work?

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d0fadfd on sbromberger:centrality into 6525b82 on JuliaLang:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6bf8019 on sbromberger:centrality into 5b4889a on JuliaLang:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 6bf8019 on sbromberger:centrality into 5b4889a on JuliaLang:master.

BSD License block

fixed bogus import

refactor, add docs

correct docs

doc fixup
@sbromberger
Copy link
Contributor Author

Closing the PR because I want to write some tests. Please, however, continue to discuss the licensing issues, as I will need direction here before proceeding.

@mlubin
Copy link
Contributor

mlubin commented Dec 30, 2014

IANAL but I don't think there are any issues with translating BSD-licensed code.

@tedsteiner
Copy link

I thought I'd just chime in and say that I would find graph centrality measures very useful. I saw you started a separate Centrality.jl package for this, but I think it would be a good fit for inclusion in Graphs.jl rather than its own repository. It seems to me that no one would ever use Centrality.jl without using Graphs.jl. But maybe the reason to separate it is because it pulls in additional dependencies (I saw you have using StatsBase)?

@sbromberger
Copy link
Contributor Author

@tedsteiner - thanks for your support. I've started my own repo because I want to work out the bugs first before an initial "Hey, it's available" announcement - I'm absolutely not opposed to including it directly in Graphs.jl, but it needs a bunch more work first and it's easier for me just to do it in a repo I've got commit privs in.

It started as a fork of Graphs but I wanted to retain the option of keeping it separate in the event the Graphs.jl folks decided against including it for whatever reason. I've designed it so that the code can easily be incorporated into Graphs.jl once/if the folks here think it's refined enough.

StatsBase is there because I needed efficient random sampling for betweenness_centrality and was too lazy to code something (that probably wouldn't work properly in any case) myself.

Please take a look at https://github.com/sbromberger/Centrality.jl and feel free to submit PRs. If you think you'd have time to write some of these, I'm happy to give committer status.

Right now I'm struggling with some inaccuracies (using networkx as the benchmark) that may be due to Float64 precision, and I'm also trying to figure out how best to optimize. Right now the code is on par with networkx but I think we should be able to make it much faster.

@sbromberger
Copy link
Contributor Author

Update: inaccuracies fixed - was an issue with multigraphs (which are not really supported via simplegraph and probably shouldn't be supported in betweenness_centrality).

I'd love some help with optimization if there's anyone who could step me through how to approach this. Profile information is at https://gist.github.com/sbromberger/6d934b78f0aaf8a4b9d5 and a juliabox notebook is at https://gist.github.com/sbromberger/10188967f0456feb3040. Also please check the README.md for other stuff if you're inclined to contribute :)

@sbromberger
Copy link
Contributor Author

@tedsteiner #155 is a compelling argument for keeping stuff separate in Centrality.jl right now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants