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

feat: Implement/fix unary minus operator -pl.col(...) #13776

Merged
merged 15 commits into from
Jan 19, 2024
Merged

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Jan 17, 2024

Closes #6192

Changes

  • Implement the Neg operator in Rust, dispatch to it from Python
  • Turn __pos__ into a no-op

Point of discussion: for unsigned integers, should we raise (like now) or should we cast it to some supertype, e.g. cast UInt8 to Int16?

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Jan 17, 2024
@stinodego stinodego marked this pull request as ready for review January 17, 2024 10:48
@orlp
Copy link
Collaborator

orlp commented Jan 17, 2024

One thing I would like to note is that overflow on negation isn't unique to the unsigned types. E.g. for Int8 you also find that -(-128) 128 (overflow) == -128.

>>> pl.Series([-1, -128], dtype=pl.Int8)
shape: (2,)
Series: '' [i8]
[
        -1
        -128
]
>>> -_
shape: (2,)
Series: '' [i8]
[
        1
        -128
]

@stinodego
Copy link
Member Author

stinodego commented Jan 17, 2024

One thing I would like to note is that overflow on negation isn't unique to the unsigned types.

I'm aware of this. In this PR it panicks with the message "attempt to negate with overflow" - there's a test for this. I think that behavior is fine as it is only the lower bound value that causes problems.

@stinodego stinodego force-pushed the fix-neg-dtype branch 2 times, most recently from c90dc76 to f6ba1a2 Compare January 18, 2024 23:16
Copy link
Member

@ritchie46 ritchie46 left a comment

Choose a reason for hiding this comment

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

Great addition @stinodego. This primitive was long overdue.

@ritchie46
Copy link
Member

One rebase away....

@stinodego
Copy link
Member Author

stinodego commented Jan 19, 2024

Rebased 👍 I just want to note that this does break any workflow that relied on doing -expr on expressions of unsigned data types. It used to auto-cast to a supertype, now it raises. Could be considered a fix, could be considered breaking. I'll let you be the judge.

@ritchie46
Copy link
Member

I consider it a fix. The previous behavior was unintentional.

@ritchie46 ritchie46 merged commit 1d434cc into main Jan 19, 2024
23 checks passed
@ritchie46 ritchie46 deleted the fix-neg-dtype branch January 19, 2024 13:30
r-brink pushed a commit to r-brink/polars that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong type promotion on lazy parquet dataframe
3 participants