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

Add log/2 and exponential/1 to Series #549

Merged
merged 6 commits into from
Mar 24, 2023

Conversation

philss
Copy link
Member

@philss philss commented Mar 24, 2023

May close #548

This version only accepts log(series, number), since Polars implements the equivalent for series.
Do you think we need log(series, series) and log(number, series)? If so, I can implement in upcoming PRs.

Also, I don't know much about exponential, so please help me documenting that.

cc/ @msluszniak @kimjoaoun

@philss philss requested a review from josevalim March 24, 2023 02:40
lib/explorer/series.ex Outdated Show resolved Hide resolved
lib/explorer/series.ex Outdated Show resolved Hide resolved
lib/explorer/series.ex Outdated Show resolved Hide resolved
lib/explorer/series.ex Outdated Show resolved Hide resolved
lib/explorer/series.ex Outdated Show resolved Hide resolved
lib/explorer/series.ex Outdated Show resolved Hide resolved
@doc type: :element_wise
@spec log(argument :: Series.t(), base :: number()) :: Series.t()
def log(argument, base) when is_number(base) do
if base <= 0, do: raise(ArgumentError, "base must be a positive number")

Choose a reason for hiding this comment

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

I think that it will be better, in general, to return negative infinity for 0 in the logarithm. However, it looks like there is no support for infinity in Explorer Series/DataFrame.

image

Copy link
Member

Choose a reason for hiding this comment

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

@msluszniak we do support it, it seems the error is elsewhere. Likely Kino.Explorer.

Choose a reason for hiding this comment

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

Oh, ok.

Choose a reason for hiding this comment

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

Yes, probably the aggregate functions like min, max, and mean don't work properly. I'll send an issue to Kino.Explorer.

Copy link
Member

Choose a reason for hiding this comment

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

The bug is in Explorer as well. Reported here: #550


"""
@doc type: :element_wise
@spec log(argument :: Series.t()) :: Series.t()
Copy link
Member

@josevalim josevalim Mar 24, 2023

Choose a reason for hiding this comment

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

Instead of having a default value, I think we should really call log/1 in the backend. This will help us avoid precision issues if we support multiple float types in the future and I believe the natural log may be more optimized. For example, it seems Rust implements all other logs are implemented on top of ln: https://doc.rust-lang.org/src/std/f64.rs.html#428-430

@philss philss merged commit 7a09c4d into elixir-explorer:main Mar 24, 2023
@philss philss deleted the ps-add-series-log branch March 24, 2023 15:28
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.

Add support for log and exp operations in DF.mutate
3 participants