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

Add pairwise convenience method for tables #123

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

nalimilan
Copy link
Member

Also add docstrings for pairwise and pairwise!.

It's great to see how simple this is thanks to Tables.jl. Better wait for JuliaData/Tables.jl#66 before merging though.

@quinnj What's the standard wording to describe "any type implementing the Tables.jl interface"? We should find a convention for it.

Also add docstrings for pairwise and pairwise!.
@quinnj
Copy link
Member

quinnj commented Feb 22, 2019

I'm actually in favor of moving away from having a way to query if something is a Table or not. I think it's best for packages to just use the fallback Any as you've coded here, or make a separate, explicit function for the Tables.jl case. Ultimately, there are a number of useful table types (generators, other unknown eltype iterators) that are perfectly valid tables, but there isn't really a way to tell if they're tables until we start trying to iterate rows.

Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

I really don't think this package should take on a Table.jl dependency. This seems like it should be in a package downstream that cares about Tables.

@nalimilan
Copy link
Member Author

I'm actually in favor of moving away from having a way to query if something is a Table or not. I think it's best for packages to just use the fallback Any as you've coded here, or make a separate, explicit function for the Tables.jl case. Ultimately, there are a number of useful table types (generators, other unknown eltype iterators) that are perfectly valid tables, but there isn't really a way to tell if they're tables until we start trying to iterate rows.

@quinnj OK. I was just asking about the best way to write the docstring, i.e. what's the standard expression to designate objects that are tables in the Tables.jl sense.

I really don't think this package should take on a Table.jl dependency. This seems like it should be in a package downstream that cares about Tables.

@KristofferC One of the big advantages of Tables.jl is that it's lightweight and generic, so adding a dependency on it doesn't cost much. To what other package could be add the few lines of code added by this PR? Creating TableDistances.jl just for that would be weird.

Alternatively we could put this under @require and expect users to either load a package that depends on Tables.jl (like DataFrames) or load it manually.

@KristofferC
Copy link
Member

One of the big advantages of Tables.jl is that it's lightweight and generic, so adding a dependency on it doesn't cost much.

> @time using Distances
  0.051232 seconds (98.17 k allocations: 5.375 MiB, 15.37% gc time)

> @time using Tables
  0.997730 seconds (2.53 M allocations: 122.008 MiB, 3.87% gc time)

I rather not increase the package load time by 20x. Just looking at the __init__ file in Tables and it is obvious it is definitely not lightweight.

This package has worked very well and been easy to maintain because it has done a simple job (vectors in, distances out) and not taking on a bunch of "lightweight generic dependencies".

@ChrisRackauckas
Copy link
Contributor

This may have changed? I don't see an __init__ in Tables.jl anymore.

@nalimilan
Copy link
Member Author

I get this now on Julia master:

julia> @time using Distances
  0.007964 seconds (8.20 k allocations: 698.664 KiB)

julia> @time using Tables
  0.017526 seconds (20.52 k allocations: 1.489 MiB)

So a dramatic improvement for both, but even more marked for Tables.

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.

4 participants