Skip to content

add allcombinations#47

Merged
bkamins merged 2 commits intomainfrom
bkamins-patch-2
Apr 24, 2022
Merged

add allcombinations#47
bkamins merged 2 commits intomainfrom
bkamins-patch-2

Conversation

@bkamins
Copy link
Member

@bkamins bkamins commented Apr 21, 2022

add allcombinations function. see JuliaData/DataFrames.jl#3031 for an example implementation.

@bkamins bkamins requested a review from nalimilan April 21, 2022 22:30
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #47 (35723d7) into main (3e905a6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #47   +/-   ##
=======================================
  Coverage   95.12%   95.12%           
=======================================
  Files           1        1           
  Lines          41       41           
=======================================
  Hits           39       39           
  Misses          2        2           
Impacted Files Coverage Δ
src/DataAPI.jl 95.12% <ø> (ø)

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 3e905a6...35723d7. Read the comment docs.

@bkamins
Copy link
Member Author

bkamins commented Apr 21, 2022

@nalimilan - now I realized that the approach proposed here (and discussed on slack) cannot be used.
The reason is that we cannot dispatch on DataFrame since its type is DataType (it is not a function).

Maybe we should leave JuliaData/DataFrames.jl#3031 as it is then? Or do you see any other solution? The only I can see is that allcombinations would define a table-like type that then would be passed to DataFrame constructor. But then we would need to provide implementation of the logic of the function in DataAPI.jl, which I think we should not do.

@nalimilan
Copy link
Member

Can't we just define methods like allcombinations(::Type{DataFrame}, ...)?

@bkamins
Copy link
Member Author

bkamins commented Apr 23, 2022

Ah - right. We can 😄. In this case can you please review this PR?

@nalimilan nalimilan requested a review from quinnj April 23, 2022 20:46
@nalimilan
Copy link
Member

@quinnj We could mention in the docstring that TableOperations owns allcombinations(NamedTuple, ...) and implement a method there.

@bkamins
Copy link
Member Author

bkamins commented Apr 23, 2022

@quinnj - could you have a look at this PR before I merge it and make a release (and finish JuliaData/DataFrames.jl#3031 a follow-up PR in DataFrames.jl)? Thank you!

@bkamins bkamins merged commit e8f7842 into main Apr 24, 2022
@bkamins bkamins deleted the bkamins-patch-2 branch April 24, 2022 06:42
@bkamins
Copy link
Member Author

bkamins commented Apr 24, 2022

Thank you!

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