Skip to content

Implement missing Python calls to make Pandas usage natural #28

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

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

liuliu
Copy link
Contributor

@liuliu liuliu commented Nov 2, 2020

This PR added implementation for PyObject_RichCompare, PyNumber_Invert,
PyNumber_And, PyNumber_Or such that the usual filtering with in pandas
would be more natural. Previously, if you do:

let pd = Python.import("pandas")
let df = pd.read_csv("a.csv")
print(df[df["column"] == match])

Will error, because we only implemented PyObject_RichCompareBool, and it
will return a simple boolean, rather than a complex PyObject for
querying. By implementing PyObject_RichCompare, PyNumber_Invert,
PyNumber_And and PyNumber_Or, I believe we enabled full range of Pandas'
filtering short-hand now:

let pd = Python.import("pandas")
let df = pd.read_csv("a.csv")
print(df[~((df["column"] == match) | (df["year"] < this_year))])

I double checked Swift type inference. If you just do:

if onePythonObject == anotherPythonObject {
}

The type system will still choose the right PyObject_RichCompareBool function.
The confusion only happens if you do:

let result = onePythonObject == anotherPythonObject

and you can workaround this by providing explicit type. I think this is a small
price to pay to enable a capable library such as Pandas usable within Swift.

This PR added implementation for PyObject_RichCompare, PyNumber_Invert,
PyNumber_And, PyNumber_Or such that the usual filtering with in pandas
would be more natural. Previously, if you do:

```swift
let pd = Python.import("pandas")
let df = pd.read_csv("a.csv")
print(df[df["column"] == match])
```
Will error, because we only implemented PyObject_RichCompareBool, and it
will return a simple boolean, rather than a complex PyObject for
querying. By implementing PyObject_RichCompare, PyNumber_Invert,
PyNumber_And and PyNumber_Or, I believe we enabled full range of Pandas'
filtering short-hand now:
```swift
let pd = Python.import("pandas")
let df = pd.read_csv("a.csv")
print(df[~((df["column"] == match) | (df["year"] < this_year))])
```
@pvieito pvieito requested review from pvieito and compnerd November 2, 2020 16:54
@pvieito
Copy link
Owner

pvieito commented Nov 2, 2020

@compnerd Hi! As this is a source-breaking change I think it would be important to have it reviewed by the Swift for TensorFlow team.

@liuliu
Copy link
Contributor Author

liuliu commented Nov 2, 2020

Thanks @pvieito @compnerd ! If we worried about ongoing source-breaking changes in the future, maybe we should review all Number Protocol: https://docs.python.org/3/c-api/number.html and decide what we should port over what we shouldn't? I can see some cases %, @ and ** is desirable, some of these will not conflict with Swift: https://docs.swift.org/swift-book/LanguageGuide/AdvancedOperators.html (~, |, ^, &).

@saeta
Copy link

saeta commented Nov 2, 2020

Thanks for flagging this @pvieito ! I defer to @marcrasi for a more detailed review, but based on my quick look over, this seems good to me. Thanks for this @liuliu!

@liuliu
Copy link
Contributor Author

liuliu commented Nov 2, 2020

Thanks @marcrasi ! Before check in, do you want me to add the rest: InplaceXor, Xor, InplaceAnd, InplaceOr?

@marcrasi
Copy link

marcrasi commented Nov 3, 2020

Thanks @marcrasi ! Before check in, do you want me to add the rest: InplaceXor, Xor, InplaceAnd, InplaceOr?

Yeah, that sounds like a good idea!

@liuliu
Copy link
Contributor Author

liuliu commented Nov 3, 2020

Done!

@marcrasi marcrasi merged commit 175e90e into pvieito:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants