-
Notifications
You must be signed in to change notification settings - Fork 8
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
Correctly decode tszip cols #213
Conversation
Pull Request Test Coverage Report for Build 11497469628Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to do this tbh - shouldn't we let tszip handle this? If we do
ts = tszip.load(tsbrowse_path)
root = zarr.open(tsbrowse_path)
# e.g.
edges_df = {
"left": ts.edges_left,
"right": ts.edges_right,
...
"extra_col_1": root["edges"]["extra_col_1"]
}
then we let tszip deal with these details, and we don't become depending on an implementation detail of tszip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor perf issue
tsbrowse/model.py
Outdated
@@ -54,44 +43,35 @@ def __init__(self, tsbrowse_path): | |||
"migrations", | |||
"provenances", | |||
]: | |||
ts_table = getattr(self.ts.tables, table_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's generating a fresh copy of the tables each time. Get a reference to the tables before the loop body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
fabcea2
to
8d4cf4c
Compare
Fixes #190
Fixed other odd columns too and added tests for all columns.