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] matrix table tail #7394

Merged
merged 6 commits into from
Nov 4, 2019
Merged

[hail] matrix table tail #7394

merged 6 commits into from
Nov 4, 2019

Conversation

iitalics
Copy link
Contributor

@iitalics iitalics commented Oct 28, 2019

stacked on #7386

  • implements .tail method on MatrixTable
    def tail(self, n: Optional[int], n_cols: Optional[int] = None) -> 'MatrixTable':
        """Subset matrix to last `n` rows.

        ....

        Parameters
        ----------
        n : :obj:`int`
            Number of rows to include (all rows included if ``None``).
        n_cols : :obj:`int`, optional
            Number of cols to include (all cols included if ``None``).
        Returns
        -------
        :class:`.MatrixTable`
            Matrix including the last `n` rows and last `n_cols` cols.
        """

question: should the interface have the same backwards compatibility naming issue as .head, to preserve consistency? or should the keyword arguments be n_cols and n_rows?

@catoverdrive
Copy link
Contributor

Can I ask you to add the same scan test here for rows and cols? This will mostly help protect against us trying to introduce an optimization that inadvertently breaks scans, which we've done in the past for e.g. filter intervals.

@iitalics
Copy link
Contributor Author

of course. added.

@iitalics
Copy link
Contributor Author

(rebased)

@iitalics iitalics removed the request for review from johnc1231 October 31, 2019 19:08

case MatrixColsHead(child, n) => lower(child, ab)
.mapGlobals('global.insertFields(colsField -> 'global (colsField).invoke("[:*]", TArray(child.typ.colType), n)))
.mapRows('row.insertFields(entriesField -> 'row (entriesField).invoke("[:*]", TArray(child.typ.entryType), n)))

case MatrixColsTail(child, n) => lower(child, ab)
.mapGlobals('global.insertFields(colsField -> 'global (colsField).invoke("[*:]", TArray(child.typ.colType), - n)))
.mapRows('row.insertFields(entriesField -> 'row (entriesField).invoke("[*:]", TArray(child.typ.entryType), - n)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what is going on here to me? Everything looks good to me other than this file, which is probably fine but confuses me because I've never looked at lowering stuff. From pattern matching against head it seems right though. I think if I understood what invoke("[*:]" meant that would clear this up enough for me to accept

Copy link
Contributor

Choose a reason for hiding this comment

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

Also have never seen that symbol literal thing before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

invoke(name, ...) creates an apply IR to call one of the functions in the registry.

[*:], [:*], and [*:*] are the functions for slicing. for instance, [*:*] gets called when you do an expression like foo[start:end] in python to get a subsequence of foo. [:*] is when you leave out the start index (so it is implicitly 0), and [*:] is when you leave out the end index (so it is implicitly the length of the array).

if an index is negative, then it means that many steps away from the end of the sequence. for instance a[-1] is an idiom in python to get the last element of a. so here, i'm doing the slice [-n:] to go from -n (i.e., n steps away from the end) to the end of the array.

>>> "hello"[-2:]
"lo"

Copy link
Contributor

@johnc1231 johnc1231 left a comment

Choose a reason for hiding this comment

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

Thanks for adding tail

@danking danking merged commit b634a24 into hail-is:master Nov 4, 2019
@iitalics iitalics deleted the mt-tail branch November 4, 2019 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants