numobs and getobs support for Tables.jl's tables

[CarloLucibello]

Thanks to the recent introduction of the subset api in Tables.jl we can now support generic tables.

Close #61, close #87

[darsnack]

Dispatching on istable is going to cause type inference issues. Maybe implement it as a separate dispatch using a traits package like suggested here.

[CarloLucibello]

Dispatching on istable is going to cause type inference issues. Maybe implement it as a separate dispatch using a traits package like suggested #61 (comment).

done, seems to work well

[codecov-commenter]

Codecov Report

Merging #124 (429062b) into main (e247fb5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   88.72%   88.77%   +0.04%     
==========================================
  Files          13       13              
  Lines         559      561       +2     
==========================================
+ Hits          496      498       +2     
  Misses         63       63              

Impacted Files	Coverage Δ
src/observation.jl	87.50% <100.00%> (+0.54%)	⬆️

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

[ablaom]

I'll take a look at this soon. One question is the rational for viewhint=false (options are true or nothing) but I am waiting for a Tables.jl query re this on slack.

[ablaom]

This looks good to me. I just wonder about the choice viewhint=false. Is this because MLUtils generally tries (requires?) copies to be made by getobs? In my own use-cases I am inclining towards avoiding the copy when that is possible (so viewhint=true).

It might be nice to allow passing the viewhint option through to getobs, but perhaps that rubs against the current interface, and in any case could be added later?

Finally, perhaps we should check if there are likely overheads from the trait dispatch, as previously flagged. I see that SimpleTraits.jl has the utility @check_fast_traitdispach. Perhaps that helps settle the issue.

[ablaom]

One final note regarding documentation. I have been chastised for equating "istable(X) = true" with "satisfies the Tables.jl interface". The implication goes only in the "=>" direction: you can have istable(X) = false for tables that implement the interface. So, that distinction should be made clear in any docs.

[CarloLucibello]

This looks good to me. I just wonder about the choice viewhint=false. Is this because MLUtils generally tries (requires?) copies to be made by getobs? In my own use-cases I am inclining towards avoiding the copy when that is possible (so viewhint=true).

getobs by design always materializes and never returns a view, we have obsview for that.

[CarloLucibello]

Finally, perhaps we should check if there are likely overheads from the trait dispatch, as previously flagged. I see that SimpleTraits.jl has the utility @check_fast_traitdispach. Perhaps that helps settle the issue.

dispatch is fast:

julia> using MLUtils, SimpleTraits, Test

julia> x = (a=rand(3), b=rand(3));

julia> @check_fast_traitdispatch MLUtils.IsTable typeof(x) true
true

[ablaom]

@CarloLucibello Thanks for those explanations and your work here.