Skip to content
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

Merged
merged 7 commits into from
Jan 5, 2017
Merged

Implemented the Moran process on graphs #799

merged 7 commits into from
Jan 5, 2017

Conversation

marcharper
Copy link
Member

@marcharper marcharper commented Jan 1, 2017

Slides here for context.

Copy link
Member

@drvinceknight drvinceknight left a 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
Copy link
Member

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?

Copy link
Member Author

@marcharper marcharper Jan 1, 2017

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, ...

Copy link
Member

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
Copy link
Member

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:

  1. 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)
  2. Can we add some tests please?
  3. 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:
    1. We allow the spatial tournament to take both a list of edges of one of these graph objects (ensures backwards compatibility);
    2. 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.

Copy link
Member

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.

Copy link
Member Author

@marcharper marcharper Jan 1, 2017

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.

Copy link
Member

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).

Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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".

Copy link
Member Author

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):
Copy link
Member

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)

Copy link
Member Author

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...

Copy link
Member

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).

Copy link
Member Author

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):
Copy link
Member

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?).

Copy link
Member Author

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.

Copy link
Member

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`.
Copy link
Member

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?

Copy link
Member Author

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.

@drvinceknight
Copy link
Member

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.
Copy link
Member

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?

@drvinceknight
Copy link
Member

There's a py3.6 failure (on the merge with master).

@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.

Thanks for this: could you cite it in the moran.rst file?

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 :)

@marcharper
Copy link
Member Author

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):
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

@meatballs meatballs merged commit 67856f9 into master Jan 5, 2017
@meatballs meatballs deleted the graph_moran branch January 5, 2017 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants