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

make Graph.migration_matrices() part of the API #309

Closed
grahamgower opened this issue Jun 1, 2021 · 3 comments
Closed

make Graph.migration_matrices() part of the API #309

grahamgower opened this issue Jun 1, 2021 · 3 comments
Milestone

Comments

@grahamgower
Copy link
Member

At the moment we have Graph._migration_matrices(), which returns a tuple (mm_list, end_times), where mm_list is a list of migration matrices and end_times is a list of the end times for each of the matrices (the initial start time for the first matrix is always inf). I recently copied this code into another project because I wanted to use it (and found a bug, hence #308). This is probably useful to others, so it would be nice to export this as part of the API.

But maybe we want to modify the interface? Or nail down what to do for edge cases, like when there are no migrations. Right now there's also no gaurantee that mm_list[i] != mm_list[i-1] --- do we care?

@apragsdale
Copy link
Member

If you've found use for it elsewhere, it's probably useful to others, so makes sense to me to export as part of the API.

If there are no migrations, it should just return a single migration matrix filled with zeros, with the single end time being 0, right?

The behavior is a bit strange to me, because it returns rows and columns in the migration matrices for demes that are not "active" over those epochs, instead of the migration matrix for only active demes. I think that's the right output for the msprime conversion, for example, but the moments methods only care about active demes over graph epochs defined by changes in parameter values. So if it's made part of the API, that should be documented pretty thoroughly I think. Also, is the row/column order of the demes guaranteed to be consistent, and should that ordered list of deme names matching the migration matrices also be returned?

@grahamgower
Copy link
Member Author

grahamgower commented Jun 1, 2021

The row/column order here is gauranteed to match the order of the demes in the graph. That ordering is the easiest to implement, and ought to be the least surprising (assuming all demes get a row/column at all times). It maps to the way msprime works, because the deme IDs/indexes are constant throught the simulation, regardless of how many other demes are alive at a given time. Likewise for SLiM, where I'm wanting to use this (just in the test code actually).

Outputing a list of migration matrices, where each matrix may have different dimensions, would then necessitate returning the list of active demes for each matrix. It would also need to add a bunch more time breakpoints to the end_times list, to explicitly account for when demes are created and go extinct. So it sounds like this function doesn't map well with the moments API. I guess you just have indexes into the SFS array, and the array changes dimensions over time?

It would be nice if we could satisfy the requirements of moments too. What does the fwdpy conversion do? Does it just use the list of Migration objects directly and not need migration matrices?

@apragsdale
Copy link
Member

I think the current behavior makes the most sense, and don't really suggest complicating it more. Was just clarifying, and I think the moments functionality is a bit of a corner case and probably less useful for the broader API. As is, I think it makes sense.

@grahamgower grahamgower added this to the 0.1.2 milestone Jun 2, 2021
grahamgower added a commit to grahamgower/demes-python that referenced this issue Jun 4, 2021
grahamgower added a commit to grahamgower/demes-python that referenced this issue Jun 4, 2021
grahamgower added a commit to grahamgower/demes-python that referenced this issue Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants