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

Add calculation of minimum spanning trees #141

Merged
merged 75 commits into from
Jan 30, 2021
Merged

Add calculation of minimum spanning trees #141

merged 75 commits into from
Jan 30, 2021

Conversation

nickjcroucher
Copy link
Collaborator

This is an inadequate implementation currrently. A design choice needs to be made about whether:

  • multiple trees should be returned if a network is not fully connected (I am not aware of a viewer of multiple MSTs since eBURST v3, so this would limit visualisation possibilities)
  • fully-connected networks could be constructed (but this is too slow for large datasets)
  • a threshold could be identified for making the network sparse but fully connected (this seems to require constructing an MST)
  • connections could be made between distinct components(either arbitrary or calculated between selected references - these could be the "founders", as identified by eBURST CCs, for which there is already code when selecting the dendropy seed_node)

@nickjcroucher nickjcroucher requested a review from johnlees January 7, 2021 16:05
@johnlees
Copy link
Member

johnlees commented Jan 8, 2021

@nickjcroucher Could you merge the master branch in?
Then I should be able to take a look on Monday

@nickjcroucher
Copy link
Collaborator Author

Will do - just for inspiration, here is the MSTree code from Grapetree, which may be helpful: https://github.com/achtman-lab/GrapeTree/blob/master/module/MSTrees.py

@johnlees
Copy link
Member

johnlees commented Jan 8, 2021

Also I'm not sure why the tests aren't running on this PR, I can try and sort that out too

@johnlees
Copy link
Member

johnlees commented Jan 8, 2021

Also I'm not sure why the tests aren't running on this PR, I can try and sort that out too

I've edited the config slightly on master, so hopefully merging master in should get the testing running on this branch

Copy link
Member

@johnlees johnlees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Changes here are mostly minor formatting/style things.

Still needed before merge:

  • Correct implementation of sparse matrix joining
  • Pin the version of dendropy in environment.yml now the midpoint rooting is fixed (and double-check NJ trees are correct, if we haven't already)
  • Add a test which runs the MST
  • Add an example to the visualisation docs
  • Add cuGraph version (see also GPU accelerated graphs #87)
  • Draw the MST as a network rather than tree (cytoscape or graphtool)
  • Alternative way of sparsifying graph (threshold/max memory approach)

I will raise a separate issue with ideas which might improve this, and we can decide whether to add this in this merge, or work on them more later

PopPUNK/network.py Outdated Show resolved Hide resolved
PopPUNK/network.py Outdated Show resolved Hide resolved
PopPUNK/network.py Outdated Show resolved Hide resolved
PopPUNK/network.py Show resolved Hide resolved
PopPUNK/network.py Outdated Show resolved Hide resolved
PopPUNK/visualise.py Outdated Show resolved Hide resolved
PopPUNK/visualise.py Outdated Show resolved Hide resolved
PopPUNK/visualise.py Outdated Show resolved Hide resolved
PopPUNK/visualise.py Outdated Show resolved Hide resolved
PopPUNK/visualise.py Outdated Show resolved Hide resolved
@johnlees johnlees linked an issue Jan 11, 2021 that may be closed by this pull request
@johnlees johnlees mentioned this pull request Jan 11, 2021
3 tasks
@johnlees johnlees linked an issue Jan 27, 2021 that may be closed by this pull request
3 tasks
@johnlees johnlees merged commit 549d74f into master Jan 30, 2021
@johnlees johnlees deleted the mst branch January 30, 2021 14:49
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

Successfully merging this pull request may close these issues.

MST improvements Specify dendropy version
2 participants