-
Notifications
You must be signed in to change notification settings - Fork 166
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
[ENH] fill_direction improve #778
[ENH] fill_direction improve #778
Conversation
update fork
update fork
Codecov Report
@@ Coverage Diff @@
## dev #778 +/- ##
==========================================
+ Coverage 94.49% 95.20% +0.70%
==========================================
Files 17 17
Lines 745 875 +130
==========================================
+ Hits 704 833 +129
- Misses 41 42 +1 |
@ericmjl @hectormz @sallyhong and others. Need your advice on the API. Currently different values for different columns cannot be applied for the limit parameter; if there are multiple columns, the same |
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.
The only change I'm requesting, @samukweku, is about docstring triple quotes, nothing else!
@@ -13,6 +13,7 @@ new version (on deck) | |||
- [ENH] Add ``pivot_wider`` function, which is the inverse of the `pivot_longer` | |||
function. @samukweku | |||
- [INF] Add `openpyxl` to `environment-dev.yml`. @samukweku | |||
- [ENH] Reduce code by reusing existing functions for fill_direction. @samukweku |
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.
🎉
# TODO: option to specify limit per column; current implementation | ||
# is one `limit` for all the columns. Might need refactoring, or an | ||
# API change. |
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.
This is a classic "do we apply this function to one column, or multiple columns" situation!
As the library matures, I've grown to realize that having singular and plural functions is not necessarily the best, so allowing a function to be applied to multiple columns simultaneously is probably the better way. There must be some way we can "compose" them together, I think, such that we get the desired behaviour. I'm kind of inspired by what I see in JAX, where we have vmap
to map a function over an array axis. This makes composing functions together more natural.
Maybe a better solution to this problem is to provide a .map_columns()
function, which allows us to map any pyjanitor function across columns. Let's talk about this in another setting. For now, I like what you've done here, @samukweku, in that you've left this API piece as a TODO to think about later on.
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.
I approve, @samukweku! Let's get this one merged in. You've done a massive job with recent PRs, please take the honour of merging this one!
PR Description
Please describe the changes proposed in the pull request:
This PR improves #704 .
PR Checklist
Please ensure that you have done the following:
<your_username>
:dev
, but rather from<your_username>
:<feature-branch_name>
.AUTHORS.rst
.CHANGELOG.rst
under the latest version header (i.e. the one that is "on deck") describing the contribution.Quick Check
To do a very quick check that everything is correct, follow these steps below:
make check
from pyjanitor's top-level directory. This will automatically run:Once done, please check off the check-box above.
If
make check
does not work for you, you can execute the commands listed in the Makefile individually.Code Changes
If you are adding code changes, please ensure the following:
$ pytest .
) locally on your machine.Documentation Changes
If you are adding documentation changes, please ensure the following:
Relevant Reviewers
Please tag maintainers to review.