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

Explorer.Series.pow does not work when supplied integers as exponent #105

Closed
kimjoaoun opened this issue Jan 27, 2022 · 9 comments · Fixed by #108
Closed

Explorer.Series.pow does not work when supplied integers as exponent #105

kimjoaoun opened this issue Jan 27, 2022 · 9 comments · Fixed by #108
Assignees
Labels
kind:bug Something isn't working

Comments

@kimjoaoun
Copy link
Contributor

Explorer.Series.pow returns an error when the user supplies an integer as exponent. It only works when exponent is a float, and I think because of that all values in the Series are casted to floating point numbers, which is not a desirable behaviour.

When exponent is a integer:

iex> s1 = [8, 16, 32] |> Explorer.Series.from_list()
#Explorer.Series<
  integer[3] 
  [8, 16, 32]
>
iex> Explorer.Series.pow(s1, 2)
** (ArgumentError) argument error
    (explorer 0.1.0-dev) Explorer.PolarsBackend.Native.s_pow(shape: (3,)
Series: '' [i64]
[
        8
        16
        32
], 1)
    (explorer 0.1.0-dev) lib/explorer/polars_backend/shared.ex:14: Explorer.PolarsBackend.Shared.apply_native/3

When exponent is a float:

iex> s1 = [8, 16, 32] |> Explorer.Series.from_list()
iex> Explorer.Series.pow(s1, 2.0)
#Explorer.Series<
  float[3]
  [64.0, 256.0, 1024.0]
>
@cigrainger
Copy link
Member

Good catch. Unfortunately this is because polars only takes f64 for the pow method: https://docs.rs/polars/0.18.0/polars/prelude/trait.SeriesTrait.html#method.pow.

I'm sure we could work around it, and at the very least document it!

@josevalim
Copy link
Member

@kimjoaoun if you could also ping polars folks and ask if they want to support an int version. It could be a good contribution upstream! ❤️

@kimjoaoun
Copy link
Contributor Author

kimjoaoun commented Jan 27, 2022

@josevalim I'll ping them!
(se eu estiver sendo chato abrindo várias issues, por favor, me avise)

@josevalim
Copy link
Member

Nah, you are being very helpful. Contributions are very welcome! :)

@kimjoaoun
Copy link
Contributor Author

kimjoaoun commented Jan 27, 2022

@josevalim It looks like pow() will be removed, so I think they'll not support an int version. I think we would need to do what ritchie46 suggested and coerce our integer to a float.

@ritchie46
Copy link

A bit of context on my rationale to not have this included on the Series trait is that this will be compiled for every datatype in polars.

Doing so for many operations will put a lot of constrain on compile times of polars-core.

The recommended way for a user to apply a numeric operation like pow, sin exp etc. is downcasting the Series to the ChunkedArray and then calling apply

Something like this.

let s = Series::new("foo", [1i32, 2, 3]);

s.i32()?.apply(|v| v.pow(3)).into_series()

Given that Explorer defines bindings to polars, you'd need to pattern match on the dtype and downcast to the appropriate type.

@cigrainger
Copy link
Member

Thanks for the clarification @ritchie46! That makes total sense as a preferred path forward.

I'm fighting against a Wednesday deadline with work at the minute but could pick this up after. Otherwise @kimjoaoun if you're keen then I can definitely give some guidance and would be happy to review a PR.

@kimjoaoun
Copy link
Contributor Author

kimjoaoun commented Jan 28, 2022

@cigrainger I accept the guidance, I'll start working on that. If I have a question should I contact you via a PR comment?

@cigrainger
Copy link
Member

cigrainger commented Jan 28, 2022

@kimjoaoun that's probably the best in terms of having a record to point to for others contributing. But you can also get me on the EEF or Elixir slacks (same username) or via email at chris at amplified dot ai.

And thanks for taking it on!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants