Skip to content

Conversation

@benjeffery
Copy link
Member

@benjeffery benjeffery commented Nov 11, 2020

Description

Displays a table-based summary of the tree sequence.

Screenshot from 2020-11-11 10-06-36

Fixes #938

PR Checklist:

  • Tests that fully cover new/changed functionality.
  • Documentation including tutorial content if appropriate.
  • Changelogs, if there are API changes.

@AdminBot-tskit
Copy link
Collaborator

📖 Docs for this PR can be previewed here

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #985 (5567ef3) into main (7ab524d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #985   +/-   ##
=======================================
  Coverage   93.70%   93.71%           
=======================================
  Files          26       26           
  Lines       20602    20622   +20     
  Branches      827      836    +9     
=======================================
+ Hits        19306    19326   +20     
  Misses       1259     1259           
  Partials       37       37           
Flag Coverage Δ
c-tests 92.45% <ø> (ø)
lwt-tests 93.57% <ø> (ø)
python-c-tests 94.96% <100.00%> (+<0.01%) ⬆️
python-tests 98.54% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/tskit/trees.py 97.27% <100.00%> (+0.01%) ⬆️
python/tskit/util.py 100.00% <100.00%> (ø)

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 7ab524d...5567ef3. Read the comment docs.

@benjeffery benjeffery marked this pull request as ready for review November 11, 2020 10:09
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.

Love it! This is super cool.



def unicode_table(title, header, rows):
if header:
Copy link
Member

Choose a reason for hiding this comment

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

if header is not None I'd say?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

return f"{name} {d}"


def unicode_table(title, header, rows):
Copy link
Member

Choose a reason for hiding this comment

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

A quick docstring would help here. (no need for full sphinx param descriptions, etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also added a docstring for the HTML method above this one.

if title is not None:
w = sum(widths) + len(rows[1]) - 1
out += [
f"╔{'═' * w}\r\n" f"║{title.ljust(w)}\r\n",
Copy link
Member

Choose a reason for hiding this comment

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

Why the \rs here? I think Python handles the universal newlines stuff and we can just put in \n.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool! Removed them all.

]
)
return util.unicode_table(
"TSKIT TreeSequence", None, ts_rows
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the "TSKIT" all-caps here, feels a bit Trumpian. I think we can just say "TreeSequence", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry been reading too much twitter. Fixed.

def __repr__(self):
ts_rows = [
["Trees", str(self.num_trees)],
["Seq Length", str(self.sequence_length)],
Copy link
Member

Choose a reason for hiding this comment

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

We can probably say "Sequence Length", I think there's plenty hspace.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@benjeffery benjeffery force-pushed the print-ts branch 3 times, most recently from 9f7af4d to ddbcbdb Compare November 11, 2020 11:53
@benjeffery
Copy link
Member Author

@jeromekelleher fixed up.

@mergify mergify bot merged commit 24861e9 into tskit-dev:main Nov 11, 2020
@petrelharp
Copy link
Contributor

Wow!!!!!!!!

@benjeffery benjeffery deleted the print-ts branch March 19, 2021 17:09
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.

make print(ts) give an informative message

4 participants