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.

Sign up for GitHub

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

Jump to bottom

Allows Series functions to receive nan, infinity, and neg_infinity values #512

Merged
merged 24 commits into from
Feb 20, 2023
Merged

Allows Series functions to receive nan, infinity, and neg_infinity values #512

merged 24 commits into from
Feb 20, 2023

Conversation

Jhonatannunessilva
Copy link
Contributor

This PR allows Series functions to receive nan, infinity, and neg_infinity values.

This is part of #467

Functions that have been tested and are working:

  • Series.equal/2
  • Series.not_equal/2
  • Series.greater/2
  • Series.greater_equal/2
  • Series.less/2
  • Series.less_equal/2
  • Series.in/2
  • Series.iotype/2
  • Series.sort/2
  • Series.argsort/2
  • Series.add/2
  • Series.subtract/2
  • Series.multiply/2
  • Series.divide/2
  • Series.pow(series, :nan | :infinity | :neg_infinity)
  • Series.pow(:nan | :infinity | :neg_infinity, series)
  • Series.pow(float_series_1, float_series_2)

NOTES:

  • The Series.pow/2 function only accepts integer series. Is it to allow float series as well?
  • In Rust, f64::NAN is not equal to f64::NAN

    • So when Series.equal/2 is called:

      iex(1)> s1 = Explorer.Series.from_list([1.0, 2.5, :nan, :infinity, :neg_infinity])
#Explorer.Series<
Polars[5]
float [1.0, 2.5, NaN, Inf, -Inf]
>

iex(2)> s2 = Explorer.Series.from_list([1.0, 3.0, :nan, :infinity, :neg_infinity])
#Explorer.Series<
Polars[5]
float [1.0, 3.0, NaN, Inf, -Inf]
>

iex(3)> Explorer.Series.equal(s1, s2)
#Explorer.Series<
Polars[5]
boolean [true, false, false, true, true]
>

    • But with the Series.in/2 function the behavior is different:

      iex(1)> s1 = Explorer.Series.from_list([1.0, 2.5, :nan, :infinity, :neg_infinity])
#Explorer.Series<
Polars[5]
float [1.0, 2.5, NaN, Inf, -Inf]
>

iex(2)> s2 = Explorer.Series.from_list([1.0, 3.0, :nan, :infinity, :neg_infinity])
#Explorer.Series<
Polars[5]
float [1.0, 3.0, NaN, Inf, -Inf]
>

iex(3)> Explorer.Series.in(s1, s2)   
#Explorer.Series<
Polars[5]
boolean [true, false, true, true, true]
>

@Jhonatannunessilva
Add Series.equal/2 tests
fda1f1a
@Jhonatannunessilva
Add Series.not_equal/2 tests
a2765fb
@Jhonatannunessilva
Add Series.greater/2 tests
ecd6cfd
@Jhonatannunessilva
Add Series.greater_equal/2 tests
1b9cb63
@Jhonatannunessilva
Add Series.less/2 tests
362c8dc
@Jhonatannunessilva
Add Series.less_equal/2 tests
a26c7e1
@Jhonatannunessilva
Update Series.in/2 test
749148d
@Jhonatannunessilva
Update Series.iotype/2 test
665361b
@Jhonatannunessilva
Add Series.sort/2 tests
dfd4c58
@Jhonatannunessilva
Add Series.argsort/2 tests
52ece85
@Jhonatannunessilva
Update Series.valid_for_bool_mask_operation?/2
50fa195 
- This function now accepts :nan, :infinity, and :neg_infinity when the Series has a numerical dtype
@Jhonatannunessilva
Add Series.add/2 tests
67866af
@Jhonatannunessilva
Add Series.subtract/2 tests
cfea516
@Jhonatannunessilva
Add Series.multiply/2 tests
ccf68e0
@Jhonatannunessilva
Add Series.divide/2 tests
09dcad3
@Jhonatannunessilva
Add Series.pow/2 tests
0562ea4
@Jhonatannunessilva
Update Series.basic_numeric_operation/3 function
de1144a 
- This function now accepts :nan, :infinity, and :neg_infinity when the Series has a numerical dtype
@Jhonatannunessilva
Update Series.add/2 function
8243188 
- This function now accepts :nan, :infinity, and :neg_infinity values
@Jhonatannunessilva
Update Series.subtract/2 function
6b1fec8 
- This function now accepts :nan, :infinity, and :neg_infinity values
@Jhonatannunessilva
Update Series.multiply/2 function
90be281 
- This function now accepts :nan, :infinity, and :neg_infinity values
@Jhonatannunessilva
Update Series.divide/2 function
ff5ce19 
- This function now accepts :nan, :infinity, and :neg_infinity values
@Jhonatannunessilva
Update Series.pow/2 function
aa38c53 
- This function now accepts :nan, :infinity, and :neg_infinity values
josevalim
josevalim reviewed Feb 20, 2023
View reviewed changes
lib/explorer/polars_backend/series.ex Outdated
@@ -193,29 +193,41 @@ defmodule Explorer.PolarsBackend.Series do
def add(%Series{} = left, %Series{} = right),
do: Shared.apply_series(left, :s_add, [right.data])

def add(left, right) when is_number(right), do: apply_scalar_on_rhs(:add, left, right)
def add(left, right) when is_number(left), do: apply_scalar_on_lhs(:add, left, right)
def add(left, right) when is_number(right) or right in [:nan, :infinity, :neg_infinity],
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using defguardp to define this? I just can't think of a good name :D

defguardp is_numerical(n) when is_number(n) or n in [:nan, :infinity, :neg_infinity]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I thought of the name is_number?/1, but it might be confused with Elixir's is_number/1. So I think the name is_numerical/1 is probably better.

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I create this guard in explorer/series.ex or polars_backend/series.ex? (It can be used in basic_numeric_operation/3 and valid_for_bool_mask_operation?/2 functions)

Copy link
Member

Choose a reason for hiding this comment

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

Explorer.Series. If you want to share, you can create it Utils, but it is small enough that should be fine to duplicate. We should also rename numeric_dtype? to is_numeric_dtype since guards have the is_ prefix instead of question marks.

Copy link
Contributor Author

@Jhonatannunessilva Jhonatannunessilva Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So should I rename the io_dtype?/1, numeric_or_bool_dtype?/1, and numeric_or_date_dtype?/1 guards as well? (is_io_dtype/1, is_numeric_or_bool_dtype/1, and is_numeric_or_date_dtype/1)

Copy link
Member

Choose a reason for hiding this comment

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

ideally yes, but since there are several, let's do that in another commit to keep this one focused. :)

@josevalim
Copy link
Member

The Series.pow/2 function only accepts integer series. Is it to allow float series as well?

Yes, we should support a series of floats too!

@josevalim josevalim merged commit 81bc0af into elixir-explorer:main Feb 20, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

I merged this for now so we can unblock work on the upcoming PRs. :)

@Jhonatannunessilva Jhonatannunessilva mentioned this pull request Feb 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josevalim josevalim josevalim left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jhonatannunessilva @josevalim