Skip to content
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: support str translate for StringMethods #10052

Merged
merged 1 commit into from
May 8, 2015

Conversation

mortada
Copy link
Contributor

@mortada mortada commented May 2, 2015

as a part of #9111

note that this handles both py2 and py3 as the signature for str.translate has changed from py2 to py3

@sinhrks @jreback

@jorisvandenbossche
Copy link
Member

I don't know if I find it a good idea to have different behaviour for python 2 vs 3. Why not just going with the python 3 signature?

@mortada
Copy link
Contributor Author

mortada commented May 2, 2015

the thing is it's more than the signature that changed from python 2 to 3. The meaning of table in str.translate also changed. If we only have the python 3 signature, users of python 2 would lose functionality with this method. In particular, users of python 2 would not be able to delete characters with StringMethods.translate, because there's no way to specify that in the python 2 structure of table.

@sinhrks
Copy link
Member

sinhrks commented May 4, 2015

Correct, but I think single internal func is better for API doc on the website to show both python 2 and 3 info. How about:

  • Merge to single function to accept both table and deletechar.
  • Ignore deletechar in python 3 with warning, and describe it in docstring.

@sinhrks sinhrks added the Strings String extension data type and string data label May 4, 2015
@jorisvandenbossche
Copy link
Member

If the change between python 2 and 3 is that big, I think even more reason for us to choose one behaviour (and thus, python 3 behaviour is of course the most logical). Users of python 2 will eventually have to change their code for python 3, so if they start using the pandas string method, they can already adapt (and otherwise they can always use the apply approach). We should follow the behaviour of the standard string methods as much as possible, but here I would personally deviate.
I would just put a warning in the docstring that it follows python 3 behaviour.

Do you know if there is a reason the behaviour changed? Did the functionality goes elwewhere?

@mortada
Copy link
Contributor Author

mortada commented May 4, 2015

so first of all, don't get me wrong - I think the python 3 signature is better and I'm a big advocate for using python 3. It's just that sticking with the python 3 signature in our implementation means that python 2 users will lose some functionality. And I'm open to making that tradeoff.

@jorisvandenbossche just to be clear, no functionality of the standard str.translate() has gone somewhere else from python 2 to python 3. The deletechars kwarg gets dropped in python 3 because the python 3 table format is flexible enough to let users specify deletions.

@jorisvandenbossche
Copy link
Member

so, python 2 users won't really lose some functionality, but just have to adapt their code somewhat?

I also understand the reasons to go for python 2/3 dependent behaviour, but as this is a 'new' feature for pandas, I would lean towards making a choice for 'one way', as we have the choice to do that

@jorisvandenbossche
Copy link
Member

@jreback @shoyer some opinion?

@mortada
Copy link
Contributor Author

mortada commented May 4, 2015

@jorisvandenbossche if we make the signature StringMethods.translate(table) for both python 2 and python 3, then users of python 2 will lose the ability to pass deletechars. Users of python 2 can't specify deletions in the python 2 table format.

@jorisvandenbossche
Copy link
Member

Ah, sorry, I didn't really look in detail to the str.translate method. I thought the table was just a normal argument, instead of a special string you can generate with a function. OK, no problem then!

@jorisvandenbossche
Copy link
Member

I think the suggestion of @sinhrks is good then, having one docstring that has information for both python 2 and 3 (as we don't have separate docs)

@shoyer
Copy link
Member

shoyer commented May 4, 2015

I haven't used str.translate before, so I don't have a strong opinion here. I think we agree that we want to be forwards compatible with Python 3 as much as possible while retaining functionality. In this case, it seems like have two function signatures is a pretty reasonable solution -- there's not too much in the way of duplicated code and it is relatively straightforward to document. But @sinhrks's solution would also be fine by me.

@jreback
Copy link
Contributor

jreback commented May 4, 2015

I agree here @sinhrks seems reasonable.

@mortada
Copy link
Contributor Author

mortada commented May 5, 2015

Sounds good thank you all for the feedback! Will update shortly.

@mortada
Copy link
Contributor Author

mortada commented May 6, 2015

updated, please take a look

def str_translate(arr, table, deletechars=None):
"""
Map all characters in the string through the given mapping table.
Equivalent to standard ``str.translate``. Note that the optional argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls use ":meth:str.translate" (and "`:func:`string.maketrans" for another) like other docstring.

@jorisvandenbossche
Copy link
Member

Can you update the docstring here as well?

@jorisvandenbossche jorisvandenbossche added this to the 0.16.1 milestone May 8, 2015
@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

@jorisvandenbossche do you mean the release note?

@jorisvandenbossche
Copy link
Member

yes, of course .. :-)

@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

yes definitely :) the only thing is that this would be on the same line as the PR for index(), rindex(), but I'll update here anyway

@jreback
Copy link
Contributor

jreback commented May 8, 2015

@mortada this was passing before yes? and you just changed docs right?

@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

yes this was passing before

@jreback
Copy link
Contributor

jreback commented May 8, 2015

@mortada ok, this needs a rebase now that I merged the other. pls ping when ready.

@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

ok cool i'll rebase

@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

ok rebase is done, updated

jreback added a commit that referenced this pull request May 8, 2015
ENH: support str translate for StringMethods
@jreback jreback merged commit 13ca328 into pandas-dev:master May 8, 2015
@jreback
Copy link
Contributor

jreback commented May 8, 2015

@mortada ok thank you!

@mortada
Copy link
Contributor Author

mortada commented May 8, 2015

sure thanks a lot!

@mortada mortada deleted the str_translate branch May 8, 2015 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Strings String extension data type and string data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants