-
Notifications
You must be signed in to change notification settings - Fork 269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented the Moran process on graphs #799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Looking forward to having this in.
Some questions (that I don't doubt you have the answer to but just want to make sure we have considered things) and subsequently a request for tests.
@@ -5,6 +5,7 @@ | |||
|
|||
# The order of imports matters! | |||
from .version import __version__ | |||
from . import graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought this line wasn't needed? Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on how you access the graph class in code, axelrod.graph.Graph
will fail without this line.
We could use from .graph import Graph, complete_graph, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on how you access the graph class in code, axelrod.graph.Graph will fail
I thought it wouldn't fail but happy as it is :)
Weighted undirected sparse graphs. | ||
|
||
Original source: | ||
/~https://github.com/marcharper/stationary/blob/master/stationary/utils/graph.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not against this. A couple of initial thoughts:
- As you have built this for your stationary library you no doubt have a reason but why we wouldn't just use networkx (which would have the quick lookups we want here)? (1 negative side is adding another dependency to the library)
- Can we add some tests please?
- The spatial tournament does not necessarily need a graph object (it just loops through the edges once via the match generator) but if we're going to have an
axelrod.graph
class we should also use that there (for consistency). I see two approaches:- We allow the spatial tournament to take both a list of edges of one of these graph objects (ensures backwards compatibility);
- We let the Moran process take a list of edges and obfuscate the building of this underlying graph object there. I think this would be my preferred approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: If we go with my suggestion of 3.i
then that would be work for a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured this would come up which is why I didn't add tests yet. We can switch this out for another library, I just used what I had laying around. It's a bit overkill since we don't need the weights (yet?), but it optimized in the right ways -- sparse implementation, precomputed neighbors. So we could also just simplify it to parts needed (relatively easy).
I'm not opposed to networkX but I'm not familiar with its API, what internal representation it uses (adjacency matrix? sparse matrix?) etc. Thoughts?
I agree re: consistent API with spatial tournaments -- we could also use isinstance
to be flexible on the input, and also have the graph's __repr__
method just output the list of edges. There are cases however that a list of edges doesn't cover, like some types of random graphs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cases however that a list of edges doesn't cover, like some types of random graphs.
That's the killer point right there! I guess that also includes the possibility of graphs that change over time etc which also rules out networkX (which really is a data representation library coupled with graph theoretic algorithms as opposed to the flexibility we might need here).
I suggest going for flexibility on the input (so we can open an issue for my suggestion of 3.i
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to get the __repr__
method to output the list of edges I expect we could use an edges
attribute or otherwise? That could be a useful attribute to have anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can save the input edges, or output a sorted list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are cases in the literature where the graph changes, it's called "active linking".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we need the graph to be weighted for some applications in the literature. If the reproduction graph is weighted then it changes the death probabilities in the birth-death case.
return graph | ||
|
||
|
||
def complete_graph(length, directed=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these extra functions? Networkx can be used to easily get a variety of graphs. (Just a thought)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed them for testing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, we can also use these in some of the tests for Spatial Tournaments (not in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to add more, an N x M grid could be useful... we can just add them as we go.
"""Returns a dictionary of the outgoing edges of source with weights.""" | ||
return self.out_mapping[source] | ||
|
||
def out_vertices(self, source): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If undirected, why not just have a adjacent_vertices
method? (To replace both the in
and out
ones?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to allow the graphs to be directed, see e.g. here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
Moran Process on Graphs | ||
----------------------- | ||
|
||
The library also provides a graph-based Moran process with `MoranProcessGraph`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to include a reference text/paper in the bibliography?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I can add a reference.
dc9ce08
to
5332637
Compare
5332637
to
0985230
Compare
There's a py3.6 failure (on the merge with master). |
@@ -26,6 +26,7 @@ documentation. | |||
.. [PRISON1998] LIFL (1998) PRISON. Available at: http://www.lifl.fr/IPD/ipd.frame.html (Accessed: 19 September 2016). | |||
.. [Robson1989] Robson, Arthur, (1989), EFFICIENCY IN EVOLUTIONARY GAMES: DARWIN, NASH AND SECRET HANDSHAKE, Working Papers, Michigan - Center for Research on Economic & Social Theory, http://EconPapers.repec.org/RePEc:fth:michet:89-22. | |||
.. [Singer-Clark2014] Singer-Clark, T. (2014). Morality Metrics On Iterated Prisoner’s Dilemma Players. | |||
.. [Shakarian2013] Shakarian, P., Roos, P. & Moores, G. A Novel Analytical Method for Evolutionary Graph Theory Problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this: could you cite it in the moran.rst file?
@marcharper I took a look at the tests and pushed a fix. I pushed directly to this PR branch but feel free to change/modify/delete if you'd rather not.
I also took the opportunity to pop the citation where I thought it should go. Only trying to be helpful so if you're not a fan please throw it out :) |
Thanks for the test fix! |
"""Returns a dictionary of the outgoing edges of source with weights.""" | ||
return self.out_mapping[source] | ||
|
||
def out_vertices(self, source): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might some of these methods be better as properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters would make sense for e.g vertices
not sure about the others. Maybe edges makes sense with a setter, essentially rewriting the graph. I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it for now. If we fancy refactoring the graph, we can do so later.
Slides here for context.