xKDR / TSFrames.jl

Timeseries in Julia
MIT License
91 stars 22 forks source link

Adding function to `join` multiple `TSFrame`s. #120

Closed codetalker7 closed 1 year ago

codetalker7 commented 1 year ago

This PR addresses issue https://github.com/xKDR/TSFrames.jl/issues/40. We have added a method for join which takes in a variable number of TSFrames (this number should be at least 2). We have assumed that the joins are left associative. To keep the syntax similar to what we already have, we have added the join type as the keyword argument jointype (since the variable number of arguments needs to be the final positional argument). For more, please refer to the discussion in https://github.com/xKDR/TSFrames.jl/issues/40.

Corresponding documentation and tests have been added.

codecov-commenter commented 1 year ago

Codecov Report

Merging #120 (70cebc9) into main (808a40a) will increase coverage by 0.11%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
+ Coverage   89.51%   89.62%   +0.11%     
==========================================
  Files          19       19              
  Lines         372      376       +4     
==========================================
+ Hits          333      337       +4     
  Misses         39       39              
Impacted Files Coverage Δ
src/join.jl 90.00% <100.00%> (+2.50%) :arrow_up:

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

chiraganand commented 1 year ago

This PR addresses issue #40. We have added a method for join which takes in a variable number of TSFrames (this number should be at least 2). We have assumed that the joins are left associative. To keep the syntax similar to what we already have, we have added the join type as the keyword argument jointype (since the variable number of arguments needs to be the final positional argument). For more, please refer to the discussion in #40.

Corresponding documentation and tests have been added.

Yes, this sounds fine. Though, my personal preference would be to reuse the on keyword (instead of jointype) used by DataFrames.jl to maintain consistency in interfaces, should be easier for users.

codetalker7 commented 1 year ago

This PR addresses issue #40. We have added a method for join which takes in a variable number of TSFrames (this number should be at least 2). We have assumed that the joins are left associative. To keep the syntax similar to what we already have, we have added the join type as the keyword argument jointype (since the variable number of arguments needs to be the final positional argument). For more, please refer to the discussion in #40. Corresponding documentation and tests have been added.

Yes, this sounds fine. Though, my personal preference would be to reuse the on keyword (instead of jointype) used by DataFrames.jl to maintain consistency in interfaces, should be easier for users.

Changed jointype to on.

chiraganand commented 1 year ago

Changed jointype to on.

On second thoughts, jointype is a more appropriate name because on in DataFrames refers to a column name and not the kind of join, the latter being referred to by the method name itself.

Can you change it back to jointype? Sorry about this.

codetalker7 commented 1 year ago

The latest commit replaces everything by a single function. Note that here we are using a for loop; we had to do this because some DataFrames join function like leftjoin and rightjoin work on only two DataFrame objects (and not more than two objects).

chiraganand commented 1 year ago

The latest commit replaces everything by a single function. Note that here we are using a for loop; we had to do this because some DataFrames join function like leftjoin and rightjoin work on only two DataFrame objects (and not more than two objects).

This should be fine as long as there are test cases to make sure the code is working fine for leftjoin et al for more than 2 or 3 TSFrame objects.

codetalker7 commented 1 year ago

The latest commit replaces everything by a single function. Note that here we are using a for loop; we had to do this because some DataFrames join function like leftjoin and rightjoin work on only two DataFrame objects (and not more than two objects).

This should be fine as long as there are test cases to make sure the code is working fine for leftjoin et al for more than 2 or 3 TSFrame objects.

Hi. Yes, we have tests for 2, 3 and 5 TSFrame objects for each function.