Skip to content

Conversation

@stefanv
Copy link
Contributor

@stefanv stefanv commented Oct 10, 2015

Proof of concept: change take to handle slicing.

See also #119 and #115

@SamLau95
Copy link
Contributor

This is a cool extension of the API! I'm concerned about documenting this for students to use. Right now Table.take is documented and the removal of that (since _Taker is private) might not allow Table.take to be documented as well.

As an aside: This functionality probably wouldn't be taught to students (who already have a lot of trouble differentiating between when to use brackets and parens).

@stefanv
Copy link
Contributor Author

stefanv commented Oct 11, 2015

We could teach the next cohort to only use square brackets for indexing
(colums, take, etc.)

Good point about the documentation, although I think I can fix that fairly
easily. I'll give it a shot and update this PR!
On Oct 11, 2015 1:00 PM, "Sam Lau" notifications@github.com wrote:

This is a cool extension of the API! I'm concerned about documenting this
for students to use. Right now Table.take is documented and the removal
of that (since _Taker is private) might not allow Table.take to be
documented as well.

As an aside: This functionality probably wouldn't be taught to students
(who already have a lot of trouble differentiating between when to use
brackets and parens).


Reply to this email directly or view it on GitHub
#120 (comment).

@mDibyo
Copy link
Contributor

mDibyo commented Oct 12, 2015

@stefanv, could you go into more detail about why you implemented a separate getitem method instead of just adding handling of slices to take.call (and sticking to the original method definition)?

Sorry if this is a stupid question. :D

@stefanv
Copy link
Contributor Author

stefanv commented Oct 12, 2015

Sure, it's because you cannot do t.take(:14).

@stefanv
Copy link
Contributor Author

stefanv commented Oct 12, 2015

Updated to alias take() to take[]

@stefanv
Copy link
Contributor Author

stefanv commented Oct 12, 2015

The docstring also seems to be fine, at least from inside IPython.

@SamLau95
Copy link
Contributor

Does Table.take still show up on the docs after running make docs?

@stefanv
Copy link
Contributor Author

stefanv commented Oct 13, 2015

Fixed the Sphinx doc.

@stefanv stefanv force-pushed the take_with_slice branch 4 times, most recently from 09d53da to e63948c Compare October 13, 2015 00:22
@stefanv
Copy link
Contributor Author

stefanv commented Oct 13, 2015

Now depends on PR #127

@SamLau95
Copy link
Contributor

Sorry for the delay on merging this! So the verdict on this is that:

  • The previous behavior of take is preserved, and we will teaching students that syntax (eg. table.take(range(5))).
  • The new behavior allows for more succinct taking of rows from a table.

To that end, could we write examples that call take using the old syntax first in the docs? Then, we can "translate them" to the indexing/slicing syntax with a note that the two are equivalent. The reason for this is that students will be looking at these docs to learn how to use the method and I'd err on the side of being clear.

@stefanv
Copy link
Contributor Author

stefanv commented Oct 20, 2015

I've updated the documentation—let me know if that is what you had in mind.

@SamLau95
Copy link
Contributor

This looks great! Thanks for the hard work on this.

SamLau95 added a commit that referenced this pull request Oct 20, 2015
POC: Allow take to handle slicing
@SamLau95 SamLau95 merged commit 9ab7674 into data-8:master Oct 20, 2015
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.

3 participants