Conversation
|
The DataAPI.jl defines |
That would be great yes. Then
How can I make |
bkamins
left a comment
bkamins
left a comment
There was a problem hiding this comment.
Now I see that what I wanted is a minimal type piracy, but I think it is OK.
You do not need to do anything more than you did. Tables.jl does not export anything, but |
Codecov Report
@@ Coverage Diff @@
## main #285 +/- ##
==========================================
- Coverage 94.88% 94.32% -0.56%
==========================================
Files 7 7
Lines 665 670 +5
==========================================
+ Hits 631 632 +1
- Misses 34 38 +4
Continue to review full report at Codecov.
|
src/fallbacks.jl
Outdated
| # get the number of rows in the incoming table | ||
| function rowcount(cols) | ||
| "Return the number of rows in the incoming table." | ||
| function nrow(cols) |
There was a problem hiding this comment.
@bkamins, does this not pirate a default definition? I'm a little uncomfortable with this definition because technically rowcount was only meant to be called on the result of Tables.columns, but now it "looks like" it works generically on any table. And for the most part, it probably will, but we're not really following the principles of the interface.
There was a problem hiding this comment.
does this not pirate a default definition?
DataAPI.jl does not define any method for this function. Same for ncol - DataAPI.jl does not define any method for this function.
As I have commented - it does pirate as it add the definition to ncol with ::Any signature. The problem is exactly what you have commented - some tables might not have a well defined row count (maybe also the same problem is for ncol). So what do you think would be best to do? What I would ideally have is some definition that works by default correctly. I think that if default errors it is not a huge problem if it does not error on "standard" tables.
Maybe what we should do is:
- leave
rowcountto be internal; - define
nrowandncolonly for tables defined in Tables.jl tables (rowtable,coltable,dictrowtable,dictcolumntable,tablemaybe some more? - you know best)
There was a problem hiding this comment.
Yes, that's what I would prefer (only define for Tables.jl-defined tables. I'm also happy to add recommendations to the docs that table types should strongly consider implementing nrow/ncol on their own table types as apart of the API.
src/fallbacks.jl
Outdated
| @deprecate rowcount(x) nrow(x) | ||
|
|
||
| "Return the number of columns in the incoming table." | ||
| ncol(cols) = length(columnnames(cols)) |
There was a problem hiding this comment.
Same here; columnnames and getcolumn are only technically allowed to be called on the result of Tables.columns or on individual iterated rows from Tables.rows, so this makes me a bit uncomfortable.
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
|
@quinnj will know best if you correctly covered everything :), but for matrices things look good :). |
4 participants
I noticed that there is a
rowcount, but not acolumncountand thatlength(columnnames(cols))was used in the codebase to count the number of columns (because that is the fastest?). This PR suggests to add acolumncountfunction.