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

[hail] Showing a negative number prints the whole table. #6226

Merged
merged 3 commits into from Jun 7, 2019

Conversation

@tpoterba
Copy link
Collaborator

@tpoterba tpoterba commented May 30, 2019

We had these semantics in our interface in the past, which was
lost in the Python reimplementation.

Fixes #6122

We had these semantics in our interface in the past, which was
lost in the Python reimplementation.

Fixes hail-is#6122
else:
rows = formatted_t.take(n + 1)
has_more = len(rows) > n
rows = rows[:n]
Copy link
Contributor

@daniel-goldstein daniel-goldstein May 30, 2019

Could you put these in a helper that returns (rows, has_more)?

@tpoterba
Copy link
Collaborator Author

@tpoterba tpoterba commented Jun 5, 2019

reassigned to @jigold since Daniel is out

@@ -1244,6 +1244,17 @@ def __repr__(self):
def _repr_html_(self):
return self.table._html_str(self.n, self.types)

def _take_n(self, n):
if n < 0:
rows = self.collect()
Copy link
Collaborator

@jigold jigold Jun 5, 2019

I think of a negative number as the equivalent of tail. Since you're collecting all of the data anyways, i think you can implement this.

Copy link
Collaborator Author

@tpoterba tpoterba Jun 5, 2019

we had these semantics before show() was rewritten in Python -- I'm reticent to change them now. I also don't want to implement showing the tail as collecting all the data -- people would try to use it and immediately run OOM.

I'm OK supporting a tail=True parameter on show() in the future, but we need to write an implementation that doesn't require localizing all the data.

Copy link
Collaborator

@jigold jigold Jun 5, 2019

In that case, can we add something to the docs that states what a negative argument does?

jigold
jigold approved these changes Jun 7, 2019
@danking danking merged commit a304358 into hail-is:master Jun 7, 2019
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants