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

Implement Series.from_list of null type #808

Merged
merged 4 commits into from
Jan 10, 2024

Conversation

lkarthee
Copy link
Member

@lkarthee lkarthee commented Jan 10, 2024

Fix for #806

Support Series.from_list([nil, nil])

Even though tests pass - DataFrame.concat_rows may fail and Series.concat will fail if one of dtypes is :null (irrespective of this fix) - need to address it later. Series concat nulls has no test cases

sn = Series.from_list([nil, nil, nil], dtype: :null)
s2 = Series.from_list([4.0, 5.0, 6.4])
Series.concat([s2, sn]) # will fail - it is valid in polars

@lkarthee
Copy link
Member Author

Switched {:f, 64} to :null when a list of nils is passed to Series.from_list/1

@@ -281,6 +280,8 @@ defmodule Explorer.Shared do
end
end)

# Logger.info("type - #{inspect type}")
# || preferable_type || {:f, 64}
type || preferable_type || {:f, 64}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can instead change this line to:

Suggested change
type || preferable_type || {:f, 64}
type || preferable_type || :null

And then we can most likely skip other changes to this file?

Copy link
Member

Choose a reason for hiding this comment

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

To be precise, skip the changes to type/2 and new_type_matches?. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

3 existing test cases fail :

s = Series.from_list([])
p = Series.cumulative_product(s)
**  (ArgumentError) Explorer.Series.cumulative_product/2 not implemented for dtype :null.
p = Explorer.Series.product(s)
**  (ArgumentError) Explorer.Series.product/1 not implemented for dtype :null
p = Series.less(s, nil)
**  cannot invoke Explorer.Series.less/2 with mismatched dtypes: {:f, 64} and nil

2 new test cases fail:

s = Series.from_list([1, 2, 3], dtype: :null)
** (ArgumentError) the value 1 does not match the inferred series dtype :null
s =
     Series.from_list([
       %{a: nil, b: nil},
       %{a: 3, b: nil},
       %{a: 5, b: nil}
     ])
** (CaseClauseError) no case clause matching: nil

There is assumption that empty array is {:f, 64}


assert Series.to_list(s) === [nil, nil, nil]
assert Series.dtype(s) == :null
end
Copy link
Member

Choose a reason for hiding this comment

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

What about a list of lists of nils?

Series.from_list([[nil, nil], [nil, nil, nil]])

Copy link
Member Author

@lkarthee lkarthee Jan 10, 2024

Choose a reason for hiding this comment

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

test/explorer/series/list_test.exs has below test case:

test "list of list of nulls" do
       series = Series.from_list([[nil, nil], [nil]])
       assert series.dtype == {:list, :null}
       assert series[0] == [nil, nil]
       assert Series.to_list(series) == [[nil, nil], [nil]]
     end

Copy link
Member

Choose a reason for hiding this comment

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

Ah, fantastic!

@josevalim josevalim merged commit eda5ac3 into elixir-explorer:main Jan 10, 2024
4 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

I will investigate the failures then, thank you!

@lkarthee lkarthee deleted the create_series_null branch January 10, 2024 09:47
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.

2 participants