Skip to content

Conversation

@winni2k
Copy link
Contributor

@winni2k winni2k commented Jan 31, 2020

This is a PR stub for the feature described in #456.

@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #457 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #457      +/-   ##
==========================================
+ Coverage   87.31%   87.32%   +<.01%     
==========================================
  Files          21       21              
  Lines       15416    15423       +7     
  Branches     2996     2998       +2     
==========================================
+ Hits        13461    13468       +7     
  Misses        969      969              
  Partials      986      986
Flag Coverage Δ
#c_tests 88.55% <100%> (ø) ⬆️
#python_c_tests 90.44% <100%> (ø) ⬆️
#python_tests 99.24% <100%> (ø) ⬆️
Impacted Files Coverage Δ
python/tskit/trees.py 98.76% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1fb5e6...3908006. Read the comment docs.

@winni2k
Copy link
Contributor Author

winni2k commented Jan 31, 2020

@jeromekelleher I think I have the basic functionality down now. Are there any tests or other features you'd like to see?

If not, then the next step is to remove all of those print statements, add some documentation and make an entry in the tutorial.

@winni2k winni2k marked this pull request as ready for review February 1, 2020 14:47
@winni2k winni2k changed the title Create Tree.as_matrix() method for conversion to networkx.DiGraph Create Tree.as_dict_of_dicts() method for conversion to networkx.Graph/DiGraph Feb 2, 2020
Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

This is AWESOME thanks @winni2k! Some comments above.

@winni2k
Copy link
Contributor Author

winni2k commented Feb 2, 2020

I've implemented all suggestions except for the Tree.as_edgelist() method. I think the dict_of_dicts approach is more flexible as it allows for branch length annotation, and I'm not sure the edge list would work with childless root nodes. See my comments above.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @winni2k. A few more minor comments and we're good to merge.

Can you squash the commits also please?

@winni2k winni2k force-pushed the networkx branch 2 times, most recently from 2af562c to 982b904 Compare February 5, 2020 10:19
@winni2k
Copy link
Contributor Author

winni2k commented Feb 5, 2020

All done

@jeromekelleher
Copy link
Member

Great, thanks @winni2k. Can you rebase off upstream/master please? The PR is a little behind, and life is easier if the git history is linear.

@winni2k
Copy link
Contributor Author

winni2k commented Feb 5, 2020

🥁

@jeromekelleher jeromekelleher merged commit a82bab1 into tskit-dev:master Feb 5, 2020
@winni2k winni2k deleted the networkx branch February 5, 2020 15:17
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.

2 participants