[ENH] Pandas String Methods in a Single Class #694

samukweku · 2020-07-03T07:12:07Z

PR Description

Please describe the changes proposed in the pull request:

Single function to access pandas' string methods
Small subset to see if the current method is acceptable

This PR resolves #360 .

PR Checklist

Please ensure that you have done the following:

PR in from a fork off your branch. Do not PR from <your_username>:dev, but rather from <your_username>:<feature-branch_name>.

If you're not on the contributors list, add yourself to AUTHORS.rst.

Add a line to CHANGELOG.rst under the latest version header (i.e. the one that is "on deck") describing the contribution.
- Do use some discretion here; if there are multiple PRs that are related, keep them in a single line.

Quick Check

To do a very quick check that everything is correct, follow these steps below:

Run the command make check from pyjanitor's top-level directory. This will automatically run:
- black formatting
- flake8 checking
- running the test suite
- docs build

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:

Ensure that you have added tests.
Run all tests ($ pytest .) locally on your machine.
- Check to ensure that test coverage covers the lines of code that you have added.
- Ensure that all tests pass.

Documentation Changes

If you are adding documentation changes, please ensure the following:

Build the docs locally.
View the docs to check that it renders correctly.

Relevant Reviewers

Please tag maintainers to review.

@ericmjl

…anitor_string_wrapper

ericmjl · 2020-07-03T20:52:07Z

@samukweku thanks for the PR! I have to admit my brain is struggling a bit to decide whether this goes in functions.py or another submodule. Can you share your thoughts here?

The other piece I'm noticing is that some of the names are inconsistent with the string methods. Would it make sense to simply wrap them verbatim?

samukweku · 2020-07-03T21:10:49Z

@ericmjl Thanks for the feedback!
Another submodule is not a bad idea. However, in functions.py we have find_replace, which has a string method attached to it, and filter_string which uses str_contains underneath. As such, I am not sure if that would create confusion.

For the names, the only one is concat, which i felt conveyed what the function was to do, compared to pandas' string cat. But, seeing that users would be more familiar with the pandas' naming, I'll stick with pandas' names.

The other thought I had, which I want feedback on, applies to chaining. An example could be:

 df.string.split(column='A', pat=',').string.get(2)

Does it look pleasing to the eye? the multiple string calls? Dont know if it is something that would put users off.

hectormz · 2020-07-04T17:40:29Z

Is this meant to handle a subset of pandas.str methods, or all of them? In the past when I looked at this issue I was intrigued by using a programmatic way to wrap them all

samukweku · 2020-07-04T17:52:52Z

@hectormz Yes, handle all of them. So, just one class to access all string methods

hectormz · 2020-07-04T18:24:20Z

Okay got it. My concern is that there are about 53 pandas.Series.str methods.
I'm torn by the manual aspect of it, and would like to see something like the programmatic/generic approach @szuckerman mentioned. But it would also need to be obvious and documented enough for users to understand what they're using.

ericmjl · 2020-07-04T20:23:05Z

@hectormz Yes, handle all of them. So, just one class to access all string methods

I agree with you, @samukweku, this should be the end goal, being able to chain up all of the string methods.

Okay got it. My concern is that there are about 53 pandas.Series.str methods.
I'm torn by the manual aspect of it, and would like to see something like the programmatic/generic approach @szuckerman mentioned. But it would also need to be obvious and documented enough for users to understand what they're using.

To make it easier to refer to, here's all of the string methods. I agree with @hectormz and @szuckerman - an automated thing might be better.

@samukweku, I think we can approach this task in a slightly smarter way than manually wrapping each function manually. This should probably be done over a few PRs, but the general idea probably would involve the following functions:

A function to identify all pandas string methods, and return their names.
A wrapper class that checks a function call for its availability as a string method, and executes the function.

From my prior experience, you might end up using the object.__dict__ representation to make this dynamic passthrough a reality. I'm not sure how deep you'll have to go with calling object.__dict__["something"].__dict__["somethingelse"] to consistently passthrough all of the functions, though, so it might take some experimenting. That said, I'm quite sure that this is a supremely fun programming problem!

Now, I just wanted to also mention that even though there may be a more automated way of approaching this task, your work thus far in this PR is still valuable, @samukweku, because it prompted the conversation and realization of how much work it would be to wrap all of the methods manually. So thank you for taking the initiative here!

samukweku · 2020-07-05T00:20:25Z

Thanks for the feedback @ericmjl @hectormz !

VPerrollaz · 2020-07-05T00:23:16Z

I personally believe that a string module might make the methods more discoverable and keep the namespaces tidy.
From a technical viewpoint, there might also be some useful stuff to pick in this for the __doc__, __name__ and __signature__ dynamic manipulation.
However this would only help users in ipython/jupyter sessions, I'm not sure how to generate static documentation with sphynx (or anything else actually) from this kind of monkey patching.

samukweku · 2020-07-09T12:10:57Z

Hi Team. Made some updates based on the suggestions. As always, love to get your feedback and suggestions for further improvement.

codecov · 2020-07-09T13:15:46Z

Codecov Report

Merging #694 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #694   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files          16       16           
  Lines         600      600           
=======================================
  Hits          559      559           
  Misses         41       41

ericmjl · 2020-07-09T19:29:20Z

Wonderful, wonderful effort, @samukweku! Thank you for handling this. I will be reviewing it this evening, after I finish my work today :).

ericmjl

Some suggested edits, @samukweku, for consistency. Otherwise, great work, thanks for doing the wrapping piece!

Let's get another review in before merging.

janitor/functions.py

ericmjl · 2020-07-10T12:28:33Z

janitor/functions.py

+    data = [
+        func.__name__
+        for _, func in inspect.getmembers(pd.Series.str, inspect.isfunction)
+        if not func.__name__.startswith("_")
+    ]
+
+    if string_function not in data:
+        raise KeyError(f"{string_function} is not a Pandas string method.")


tests/functions/test_process_text.py

hectormz

Thanks @samukweku ! I echo Eric.

I'll take his docstring requests one small step further by asking for periods at the end of each.

Currently, pandas will raise an exception if an invalid kwarg is provided right? Should we leave that as is, or add our JanitorError into the mix as well?

I'm assuming that the tests are the same ones you made when you started to implement this manually. Do you think they are sufficient, or are there other tests for other functions that would cover some corner cases? It would be too much to test them all, but just a thought that came to mind.

samukweku · 2020-07-10T20:38:41Z

@ericmjl @HectorM14 Thanks! got some guidance from @VPerrollaz script, so it's a community effort :) I will add more tests, and possibly check Stack Overflow or some blogs for some edge cases. Not familiar with JanitorError, so cant say much on that; I default to Pandas exception handling. As always, more feedback are welcome.

hectormz · 2020-07-10T20:47:07Z

@samukweku You don't have to add new tests. I was just curious if they were picked for a reason, or if there is an ideal set that covers different issues.

Do all the string methods return a series?

samukweku · 2020-07-10T20:49:22Z

@HectorM14 , yes, all the string methods return a series, same as in Pandas. The tests were picked at random, just testing for those that needed extra parameters and just one test for string methods that required no parameters

hectormz · 2020-07-13T04:39:06Z

mamba IRL! 🤩

janitor/functions.py

tests/functions/test_process_text.py

hectormz · 2020-07-13T05:20:13Z

If the conda/mamba solving is fixed, I just added some requested changes to tighten up the docstrings and it's all good to go @samukweku !

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

samukweku · 2020-07-13T05:36:31Z

Yes, problem fixed. @hectormz @ericmjl how did you know what to fix for the build error? New to this, so questions might be silly. When I checked the view log, the only error I noticed was deploy.provider : unknown value pages:git.

samukweku · 2020-07-13T06:01:02Z

@ericmjl Also, how do I fix the deepsource error?

ericmjl · 2020-07-13T09:48:51Z

@samukweku when I looked at the build log, I saw that only that particular build stalled at the docker container build step in which the conda env solving piece looked slow.

As such, I had the following hypotheses:

Something went wrong with conda
Something went wrong with docker build

At first, I thought it was conda stalling. In writing the response to you, I then realized, oh, the first conda step actually worked, so maybe the docker build was the real issue. But I wasn’t sure how to debug what’s going on in a Docker container build on a remote machine, so I thought, let’s just disable it for now, since it’s not 100% crucial for the package. It’s merely helpful for development.

That was my thought process! Hopefully that’s informative for you too :).

ericmjl · 2020-07-13T09:51:32Z

As for the current errors, let’s see.

For DeepSource: does the deep source config file (.deepsource.toml) exist in your fork?

For the Azure builds, looks like one was an HTTP error. That just needs a restart. I think I have added you to the pipelines, @samukweku. Can you see if you can restart the build? (You might need to login to Azure again.)

samukweku · 2020-07-13T10:53:12Z

Didn't have to do anything except push new commit. Fingers crossed.

ericmjl · 2020-07-13T14:10:46Z

Everything looks good to me. @hectormz would you like to do the honors?

hectormz · 2020-07-13T15:54:20Z

@ericmjl will do! just fixed one last thing in .rst's. Will wait for the CI to finish and then will merge!

ericmjl · 2020-07-13T20:11:29Z

🎉

root added 5 commits June 24, 2020 22:02

Updated general_functions API page

bb73f91

Updated changelog

0f4391a

String Method functions

c0152f7

Merge branch 'dev' of https://github.com/samukweku/pyjanitor into pyj…

6ca95a5

…anitor_string_wrapper

updates to string methods

f69d1cf

root added 2 commits July 9, 2020 08:21

updates to pyjanitor string methods

19d9448

update string methods

e6ad2c5

samukweku requested review from szuckerman, ericmjl, zbarry and hectormz July 9, 2020 12:16

root added 3 commits July 9, 2020 12:17

updates to string methods

c346683

update to string methods

7095dd7

updates to string methods

3a064bc

ericmjl requested changes Jul 10, 2020

View reviewed changes

hectormz requested changes Jul 10, 2020

View reviewed changes

root added 2 commits July 13, 2020 04:11

changes to travis.yml

9b98f92

updates to string methods

a6f6c4c

hectormz requested changes Jul 13, 2020

View reviewed changes

samukweku and others added 7 commits July 13, 2020 15:22

Update tests/functions/test_process_text.py

b6d4b57

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

Update tests/functions/test_process_text.py

320f9fe

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

Update tests/functions/test_process_text.py

64577f6

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

Update tests/functions/test_process_text.py

5c92e82

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

Update janitor/functions.py

6eda096

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

Update janitor/functions.py

6b8d303

Co-authored-by: Hector <23343812+hectormz@users.noreply.github.com>

updates to string methods

230ab34

updates to changelog

0540a5d

root and others added 2 commits July 13, 2020 10:39

updates to changelog

4422cf2

Merge branch 'dev' into pyjanitor_string_wrapper

b54c142

hectormz added 2 commits July 13, 2020 08:49

Update CHANGELOG.rst

c76eba3

Update general_functions.rst

66334e5

hectormz approved these changes Jul 13, 2020

View reviewed changes

hectormz merged commit fb56198 into pyjanitor-devs:dev Jul 13, 2020

samukweku deleted the pyjanitor_string_wrapper branch July 13, 2020 20:59

samukweku self-assigned this Nov 28, 2020

samukweku mentioned this pull request Dec 4, 2020

[ENH] Process text improve #779

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Pandas String Methods in a Single Class #694

[ENH] Pandas String Methods in a Single Class #694

samukweku commented Jul 3, 2020 •

edited

ericmjl commented Jul 3, 2020

samukweku commented Jul 3, 2020

hectormz commented Jul 4, 2020

samukweku commented Jul 4, 2020

hectormz commented Jul 4, 2020

ericmjl commented Jul 4, 2020 •

edited

samukweku commented Jul 5, 2020

VPerrollaz commented Jul 5, 2020

samukweku commented Jul 9, 2020 •

edited

codecov bot commented Jul 9, 2020 •

edited

ericmjl commented Jul 9, 2020

ericmjl left a comment •

edited

ericmjl Jul 10, 2020

hectormz left a comment •

edited

samukweku commented Jul 10, 2020

hectormz commented Jul 10, 2020

samukweku commented Jul 10, 2020

hectormz commented Jul 13, 2020

hectormz commented Jul 13, 2020

samukweku commented Jul 13, 2020

samukweku commented Jul 13, 2020

ericmjl commented Jul 13, 2020

ericmjl commented Jul 13, 2020

samukweku commented Jul 13, 2020

ericmjl commented Jul 13, 2020

hectormz commented Jul 13, 2020

ericmjl commented Jul 13, 2020

[ENH] Pandas String Methods in a Single Class #694

[ENH] Pandas String Methods in a Single Class #694

Conversation

samukweku commented Jul 3, 2020 • edited

PR Description

PR Checklist

Quick Check

Code Changes

Documentation Changes

Relevant Reviewers

ericmjl commented Jul 3, 2020

samukweku commented Jul 3, 2020

hectormz commented Jul 4, 2020

samukweku commented Jul 4, 2020

hectormz commented Jul 4, 2020

ericmjl commented Jul 4, 2020 • edited

samukweku commented Jul 5, 2020

VPerrollaz commented Jul 5, 2020

samukweku commented Jul 9, 2020 • edited

codecov bot commented Jul 9, 2020 • edited

Codecov Report

ericmjl commented Jul 9, 2020

ericmjl left a comment • edited

Choose a reason for hiding this comment

ericmjl Jul 10, 2020

Choose a reason for hiding this comment

hectormz left a comment • edited

Choose a reason for hiding this comment

samukweku commented Jul 10, 2020

hectormz commented Jul 10, 2020

samukweku commented Jul 10, 2020

hectormz commented Jul 13, 2020

hectormz commented Jul 13, 2020

samukweku commented Jul 13, 2020

samukweku commented Jul 13, 2020

ericmjl commented Jul 13, 2020

ericmjl commented Jul 13, 2020

samukweku commented Jul 13, 2020

ericmjl commented Jul 13, 2020

hectormz commented Jul 13, 2020

ericmjl commented Jul 13, 2020

samukweku commented Jul 3, 2020 •

edited

ericmjl commented Jul 4, 2020 •

edited

samukweku commented Jul 9, 2020 •

edited

codecov bot commented Jul 9, 2020 •

edited

ericmjl left a comment •

edited

hectormz left a comment •

edited