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

Fix mismatched types in Series.pow #821

Merged
merged 20 commits into from
Jan 16, 2024
Merged

Conversation

billylanchantin
Copy link
Contributor

@billylanchantin billylanchantin commented Jan 14, 2024

Fixes:

New dtype handling as of Rust Polars 0.36.2:

pow(uint,  uint)  = uint
pow(uint,  sint)  = float
pow(sint,  uint)  = sint
pow(sint,  sint)  = float
pow(float, *int)  = float
pow(*int,  float) = float
pow(float, float) = float

native/explorer/src/series.rs Outdated Show resolved Hide resolved
test/explorer/series_test.exs Show resolved Hide resolved
@billylanchantin
Copy link
Contributor Author

Hey sorry for the churn! I was playing with a version that kept everything in Elixir but I didn't mean to push it yet.

@billylanchantin
Copy link
Contributor Author

Ok I think this new Elixir-only direction is better.

  1. We're more in control of the types. @josevalim, it should now be easy to follow Nx if we want (we may need a post-cast in some cases though).
  2. It helped catch some inconsistencies in other tests.
  3. Less Rust code for me to mess up. 😛

lib/explorer/series.ex Outdated Show resolved Hide resolved
Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Smart use of DFs!

Copy link
Member

@philss philss left a comment

Choose a reason for hiding this comment

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

Looks great! Nice usage of the DF, and nice way to test it! :D

But I want to document it before we merge. Otherwise anyone coming in with Polars expectations will be thrown off.

The idea here is to document for the end users, right? I'm 👍 for this.

@billylanchantin
Copy link
Contributor Author

Thanks, y'all!

The idea here is to document for the end users, right? I'm 👍 for this.

@philss Yeah exactly. Raising on pow(unsigned, signed) is a totally sensible choice, but it differs from what Polars is doing (and Pandas I think) so I want to call it out in the docs.

@billylanchantin
Copy link
Contributor Author

Note: I misunderstood what @josevalim was getting at with his suggestion above. He was only consolidating the cases, not making pow work like it does in Nx.

We can still do that if we want, but perhaps that should be a separate PR. If so, I think this one is good to go.

Give this comment a 👍 if we're good with that and I'll merge it. (Or someone can just merge it themselves 😄)

@billylanchantin billylanchantin merged commit 8fa5071 into main Jan 16, 2024
4 checks passed
@billylanchantin billylanchantin deleted the bl-pow-eager-and-lazy branch January 16, 2024 22:49
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