add getrows #284

CarloLucibello · 2022-06-28T06:27:30Z

@quinnj @bkamins @nalimilan @ablaom @ToucheSir

src/Tables.jl

bkamins · 2022-06-28T07:23:12Z

This PR should probably provide appropriate implementations for rowtable, columntable, dictrowtable, dictcolumntable, table as these are defined in Tables.jl (an additional benefit of doing this will be that package developers will see how to properly implement getrows for their own types).

Also probably it would be good to bump version number so that it can be released after merging.

codecov · 2022-06-28T07:25:37Z

Codecov Report

Merging #284 (76f1375) into main (60c080c) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
+ Coverage   94.88%   94.94%   +0.06%     
==========================================
  Files           7        7              
  Lines         665      673       +8     
==========================================
+ Hits          631      639       +8     
  Misses         34       34

Impacted Files	Coverage Δ
src/Tables.jl	`88.00% <100.00%> (ø)`
src/namedtuples.jl	`100.00% <100.00%> (ø)`
src/matrix.jl	`100.00% <0.00%> (ø)`
src/fallbacks.jl	`98.60% <0.00%> (+<0.01%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

CarloLucibello · 2022-06-28T07:29:16Z

This PR should probably provide appropriate implementations for rowtable, columntable, dictrowtable, dictcolumntable, table as these are defined in Tables.jl (an additional benefit of doing this will be that package developers will see how to properly implement getrows for their own types).

I'm totally unfamiliar with this package so I was hoping to just get the stub in to get things started. I can have a look around and see if I can manage to write those implementations as well.

bkamins · 2022-06-28T07:36:33Z

I can have a look around and see if I can manage to write those implementations as well.

These should not be that hard. Of course we can do it later.

@quinnj, @nalimilan - do you think we can have some default fallback implementation of this method?

src/Tables.jl

quinnj · 2022-06-30T03:15:02Z

src/Tables.jl

+Return one or more rows from table `x` according to the position(s) specified by `inds`:
+
+- If `inds` is a single integer return a row object.
+- If `inds` is a collection of integers, return a table object. 


Suggested change

- If `inds` is a collection of integers, return a table object.

- If `inds` is a collection of integers, return an indexable object of rows

Just a bit more specific; one of the main motivations for this was to ensure users can get an in-memory/indexable collection of rows in a consistent way.

Are you sure we want to require Tables.getrows to return a indexable collection of rows? That would mean that e.g. for a NamedTuple of vectors getrows couldn't return another NamedTuple of vectors, even if that's the most efficient representation. IOW this is against the implementation that this PR adds for ColumnTable. :-)

src/namedtuples.jl

quinnj

This is a great start; thanks @CarloLucibello. Sorry I've been so slow on getting around to this or responding on the original issue. Just a little cleanup here and if you'll add some tests, I think it's ready to go. Once we merge, I can take a look at doing the implementation for the other table types in Tables.jl.

CarloLucibello · 2022-06-30T06:48:18Z

Done! Can you unlock CI?

CarloLucibello · 2022-06-30T15:20:07Z

good to go?

src/Tables.jl

nalimilan · 2022-07-01T20:27:30Z

src/Tables.jl

+Return one or more rows from table `x` according to the position(s) specified by `inds`:
+
+- If `inds` is a single integer return a row object.
+- If `inds` is a collection of integers, return a table object. 


Are you sure we want to require Tables.getrows to return a indexable collection of rows? That would mean that e.g. for a NamedTuple of vectors getrows couldn't return another NamedTuple of vectors, even if that's the most efficient representation. IOW this is against the implementation that this PR adds for ColumnTable. :-)

nalimilan · 2022-07-01T20:44:08Z

@quinnj, @nalimilan - do you think we can have some default fallback implementation of this method?

Indeed, why not have getrows return a RowTable for row-oriented tables and a ColumnTable for column-oriented tables? For row-oriented tables, a simple solution would be to call Tables.rows, ensure the result is a vector (and if not call collect on it), and then call getindex or view on it. For column-oriented tables, call Tables.columns, and for each column ensure it is a vector (calling collect on it if necessary) and call getindex or view on it. In both cases, instead of calling collect it would be better in term of memory use to allocate a vector of the final size and fill it on the fly, but it's more tricky to do so maybe not worth it for a first fallback implementation.

EDIT: actually what I described is just calling getrows(Tables.rowtable(tbl), ...)) when Table.isrowtable(tbl) and getrows(Tables.columntable(tbl), ...)) otherwise. So that's quite trivial to implement.

ablaom · 2022-08-02T21:50:12Z

@quinnj 🙏🏾

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

quinnj

This LGTM; @bkamins, can I ask you to take one more look here before merging? Sorry to let this sit for so long; I've been tied up with a lot f stuff.

src/Tables.jl

bkamins · 2022-08-28T14:42:37Z

I have left one comment about allowed inds + Tables.jl version needs to be bumped.

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

nalimilan · 2022-08-30T07:47:11Z

@quinnj What about throwing an error as discussed above?

bkamins · 2022-08-30T08:50:47Z

I agree with @nalimilan that it is better to throw an error - at least for the reference implementations (so users, by looking at the code can see what is expected).

As I have commented above - I think the simplest thing to check is if the returned object is RowTable or ColumnTable respectively.

quinnj · 2022-09-05T22:27:26Z

Ok, but here's an issue I've found digging into this further (and introducing the error checks mentioned); the current implementation of getrows for ColumnTable doesn't follow the getrows docs, since it returns a ColumnTable instead of return an indexable object of rows..

The point of this is we always get either a single row or collection of rows, right? Even if the input is column-oriented? Or should we update the docs and say if inds is a collection, you get a subset of original input, row or column oriented.

quinnj · 2022-09-05T22:39:29Z

And indeed, @nalimilan pointed this exact issue out here.

One idea is that we could provide a generic ColumnsRows struct that would basically be a wrapper for any result of Tables.columns that provided an "indexable collection of rows" interface to the underlying columns (essentially iterating ColumnsRow that we already use in the generic Tables.rows fallback for column oriented tables).

I might take a stab at this approach and see what it looks like.

bkamins · 2022-09-06T06:11:35Z

Or should we update the docs and say if inds is a collection, you get a subset of original input, row or column oriented.

This is what I think we should do. For example for DataFrames.jl I imagine that we will have the following implementation:

Tables.getrows(df::AbstractDataFrame, inds; view=nothing) = view === true ? view(df, inds, :) : df[inds, :]

as it seems most natural (and in general - it will be more efficient for a given table type to decide what to do; I assume that people will expect Tables.getrows to be efficient).

I would set the issue of "indexable collection of rows" and "indexable collection of columns" (for completeness as separate case) since it is orthogonal to the Tables.getrows design. I think we should provide such an API (again, in DataFrames.jl this API is provided by the eachrow and eachcol functions, but we could choose some other names here, as in Base Julia eachrow and eachcol were added later and they only support iteration interface but not indexing interface).

quinnj · 2022-09-06T16:08:36Z

So given that we want this to be more of a general "subset" function, I'd like to change the name from getrows. 1) I think Tables.getrows is too similar to Tables.rows and it seems hard to explain the difference since they seem so similar and 2) for column-oriented tables, you're not really getting "rows" back, but you're just getting a subset of the original table.

I'm going to propose we call this Tables.subset to be more clear.

bkamins · 2022-09-06T16:13:46Z

Tables.subset is very good for me as it is consistent in terminology with DataFrames.jl :).
@ablaom @ToucheSir @CarloLucibello - are you OK with this?

Alternatively we could have 2 functions:

Table.getrow to get a single row (and it would accept a single index)
Table.subset to get a subset of a table and return a table (and it would expect a collection of indices)

quinnj · 2022-09-06T16:35:04Z

Ok, cleaned up implementation here: #292

CarloLucibello · 2022-09-06T17:02:05Z

Tables.subset is a reasonable choice, fine for me

ablaom · 2022-09-06T21:43:42Z

Mmm. I'm not super happy with removing "rows" from the the name. If I'm new to tables in julia, "subset" could mean anything. I similarly never liked name like "select" . Select what? I want to know which axis I'm slicing on.

How about rowsubset ?

quinnj · 2022-09-06T22:28:35Z

But that's kind of the point of our discussion/decision above: this function isn't necessarily returning "rows" if the original table input is column-oriented; rather, it returns a "subset" of the original table, preserving whether the original table was column or row-oriented. Thus, Tables.subset (note the Tables. prefix!) seems clearest to what the function is actually doing.

ablaom · 2022-09-06T23:32:21Z

Yes, I get that it is not returning "rows" in the sense of not returning a row iterator. But I'm not sure general users equate "rows" with "row iterator", maybe just Tables.jl developers do that 😉 .

I also appreciate these name decisions are very difficult to get right and to get consensus on. Just putting in my two cents.

And I agree that aligning with DataFrames - which most users will know well - makes sense.

ToucheSir · 2022-09-07T14:58:57Z

One could always go maximally explicit with something like Tables.subset_along_row_dim, but I imagine folks would not be terribly enthused about that name :P

add getrows

9bc355c

CarloLucibello mentioned this pull request Jun 28, 2022

Can MLUtils play nicely with Tables.jl? JuliaML/MLUtils.jl#61

Closed

bkamins reviewed Jun 28, 2022

View reviewed changes

src/Tables.jl Outdated Show resolved Hide resolved

implement getrows for RowTable and ColumnTable

b41146a

CarloLucibello force-pushed the main branch from 8869ec2 to b41146a Compare June 28, 2022 13:13

quinnj reviewed Jun 30, 2022

View reviewed changes

src/Tables.jl Outdated Show resolved Hide resolved

quinnj reviewed Jun 30, 2022

View reviewed changes

src/namedtuples.jl Outdated Show resolved Hide resolved

quinnj reviewed Jun 30, 2022

View reviewed changes

src/namedtuples.jl Outdated Show resolved Hide resolved

quinnj requested changes Jun 30, 2022

View reviewed changes

CarloLucibello added 2 commits June 30, 2022 06:59

add tests and implement feedbacks from review

04c4991

ignore vscode

6695658

nalimilan reviewed Jul 1, 2022

View reviewed changes

Update src/Tables.jl

c539d5a

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

quinnj approved these changes Aug 28, 2022

View reviewed changes

bkamins reviewed Aug 28, 2022

View reviewed changes

src/Tables.jl Outdated Show resolved Hide resolved

bkamins mentioned this pull request Aug 28, 2022

add Tables.subset and move ByRow to Tables JuliaData/DataFrames.jl#3127

Closed

Update src/Tables.jl

76f1375

Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>

quinnj merged commit d7cea3b into JuliaData:main Aug 30, 2022

ablaom mentioned this pull request Sep 6, 2022

Change getrows -> subset #292

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add getrows #284

add getrows #284

CarloLucibello commented Jun 28, 2022 •

edited

Loading

bkamins commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

CarloLucibello commented Jun 28, 2022

bkamins commented Jun 28, 2022

quinnj Jun 30, 2022

nalimilan Jul 1, 2022 •

edited

Loading

quinnj left a comment

CarloLucibello commented Jun 30, 2022

CarloLucibello commented Jun 30, 2022

nalimilan Jul 1, 2022 •

edited

Loading

nalimilan commented Jul 1, 2022 •

edited

Loading

ablaom commented Aug 2, 2022

quinnj left a comment

bkamins commented Aug 28, 2022

nalimilan commented Aug 30, 2022

bkamins commented Aug 30, 2022

quinnj commented Sep 5, 2022

quinnj commented Sep 5, 2022

bkamins commented Sep 6, 2022

quinnj commented Sep 6, 2022

bkamins commented Sep 6, 2022

quinnj commented Sep 6, 2022

CarloLucibello commented Sep 6, 2022

ablaom commented Sep 6, 2022

quinnj commented Sep 6, 2022

ablaom commented Sep 6, 2022

ToucheSir commented Sep 7, 2022

	- If `inds` is a collection of integers, return a table object.
	- If `inds` is a collection of integers, return an indexable object of rows

add getrows #284

add getrows #284

Conversation

CarloLucibello commented Jun 28, 2022 • edited Loading

bkamins commented Jun 28, 2022 • edited Loading

codecov bot commented Jun 28, 2022 • edited Loading

Codecov Report

CarloLucibello commented Jun 28, 2022

bkamins commented Jun 28, 2022

quinnj Jun 30, 2022

Choose a reason for hiding this comment

nalimilan Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

quinnj left a comment

Choose a reason for hiding this comment

CarloLucibello commented Jun 30, 2022

CarloLucibello commented Jun 30, 2022

nalimilan Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

nalimilan commented Jul 1, 2022 • edited Loading

ablaom commented Aug 2, 2022

quinnj left a comment

Choose a reason for hiding this comment

bkamins commented Aug 28, 2022

nalimilan commented Aug 30, 2022

bkamins commented Aug 30, 2022

quinnj commented Sep 5, 2022

quinnj commented Sep 5, 2022

bkamins commented Sep 6, 2022

quinnj commented Sep 6, 2022

bkamins commented Sep 6, 2022

quinnj commented Sep 6, 2022

CarloLucibello commented Sep 6, 2022

ablaom commented Sep 6, 2022

quinnj commented Sep 6, 2022

ablaom commented Sep 6, 2022

ToucheSir commented Sep 7, 2022

CarloLucibello commented Jun 28, 2022 •

edited

Loading

bkamins commented Jun 28, 2022 •

edited

Loading

codecov bot commented Jun 28, 2022 •

edited

Loading

nalimilan Jul 1, 2022 •

edited

Loading

nalimilan Jul 1, 2022 •

edited

Loading

nalimilan commented Jul 1, 2022 •

edited

Loading