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

map(f, t): create Columns when f returns tuple / named tuple #54

Closed
wants to merge 2 commits into from

Conversation

shashi
Copy link
Collaborator

@shashi shashi commented Jul 10, 2017

Turned out to be a bit complicated...

addresses #52

@codecov-io
Copy link

codecov-io commented Jul 10, 2017

Codecov Report

Merging #54 into master will decrease coverage by 0.37%.
The diff coverage is 81.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   93.49%   93.11%   -0.38%     
==========================================
  Files           6        6              
  Lines         922      959      +37     
==========================================
+ Hits          862      893      +31     
- Misses         60       66       +6
Impacted Files Coverage Δ
src/IndexedTables.jl 91.48% <81.57%> (-3.71%) ⬇️
src/columns.jl 96.12% <0%> (+0.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01dde0b...d2e7a4f. Read the comment docs.

@davidanthoff
Copy link
Contributor

Should a map that returns a Pair{NamedTuple{...}, NamedTuple{...}} return an IndexedTable where the columns in the key of the pair make up the index, and the columns in the value make up the data columns? This would essentially be #55, but it would use a normal map as the API.

In some sense, all we would need is a constructor for IndexedTable that takes an iterator of that element type. That is something I wanted to add for some while: queryverse/IterableTables.jl#26

@shashi
Copy link
Collaborator Author

shashi commented Jul 10, 2017

Should a map that returns a Pair{NamedTuple{...}, NamedTuple{...}} return an IndexedTable where the columns in the key of the pair make up the index, and the columns in the value make up the data columns?

I don't think so... map should only work on values. You could have output keys that collide allowing you to implement filter with map which seems wrong.

a constructor for IndexedTable that takes an iterator of that element type

Good idea to add this... One could even convert any Associative to an IndexedTable just by calling this constructor.

@shashi
Copy link
Collaborator Author

shashi commented Jul 11, 2017

An alternative approach using Base.promote_op is in #57

@shashi
Copy link
Collaborator Author

shashi commented Jul 12, 2017

t = IndexedTable([1,2,3], Columns(x=[4,5,6]))
y = [1, 1//2, "x"]
t4 = map(x->(x.x, y[x.x-3]), t)

This doesn't work with the alternative approach as promote_op has difficulty figuring out the output type. Creating a function with return type annotation also didn't work...

@shashi
Copy link
Collaborator Author

shashi commented Jul 12, 2017

has difficulty figuring out the output type

Turns out it was throwing away the fact that the results are tuples when any field is UnionAll... Fixed in #57

@shashi shashi closed this Jul 12, 2017
@shashi shashi deleted the s/map branch July 12, 2017 18:14
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