-
Notifications
You must be signed in to change notification settings - Fork 54
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
New function to combine feature dataframes #186
New function to combine feature dataframes #186
Conversation
I should note that the original idea for this came from some code that @JuliaKukulies posted that I found a couple years ago on #17. |
Codecov ReportBase: 35.06% // Head: 35.58% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.4.0 #186 +/- ##
=============================================
+ Coverage 35.06% 35.58% +0.52%
=============================================
Files 10 10
Lines 2099 2116 +17
=============================================
+ Hits 736 753 +17
Misses 1363 1363
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition, @freemansw1 ! The function is very well thought out, and I have tested it by splitting the test data sets. It's working flawlessly. All of my comments are just some minor details, therefore I'm approving this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks for taking care of this @freemansw1! Your code works well and the documentation is clear and helpful! I have only a small question. Otherwise, nothing to add.
Ok, since I've added new functionality, will wait for @JuliaKukulies and @snilsn to confirm (and CI to pass) before merging. Thanks to both of you for your very quick reviews! |
No concerns from my side @freemansw1 |
Not from my side either. Please go ahead and merge @freemansw1 And thank you very much for the addition, I find it useful! |
This should address #17 (!) and provides a function for combining multiple, independently identified feature dataframes into a single dataframe appropriate for tracking.