Add Series.strptime/2

[anthony-khong]

Closes #625

I would be happy to work on strftime after this!

[josevalim]

One comment and we can ship it. <3

[anthony-khong]

Hi @josevalim, thank you so much for reviewing the PR quickly. I've added a few changes, so if it's okay, could you please re-review the PR?

Your comment made me wonder how exactly the formats differ from Calendar.strftime's format. I spent more time studying Polars' codebase, so I think I understand Polars' strptime a bit better. Here's a summary of the changes since you last saw the PR:

• It turns out that we can avoid iterating in series.rs by using as_datetime (instead of as_datetime_not_exact. However, it looks like there's a slight change in the function signature between Polars' current main branch and v0.29 - the utc parameter is gone. This implementation probably needs to be changed when Explorer upgrades to v0.30.
• Add tests to confirm that it's compatible to Chrono's format. In particular, verify formats not covered by Calendar.strftime here.
• I also noticed that Polars can do format inference, so I added a nil default for the format_string argument and a test to use the inferred format.

I hope the additions are helpful, but if not, I'm happy to undo some of the changes!

[josevalim]

If you want to cast a string to datetime (i.e. nil), you call cast(:datetime). So I think we can remove nil and we document that cast(:date) and cast(:datetime) are also options. :)

[anthony-khong]

Hi @josevalim, thank you again for the feedback. That makes total sense. I've removed the possibility of nil format.

[josevalim]

Suggested change

	Converts a string series to a datetime series.
	For the format string specification, refer to the [chrono crate documentation](https://docs.rs/chrono/latest/chrono/format/strftime/index.html).
	Converts a string series to a datetime series with a given `format_string`.
	For the format string specification, refer to the
	[chrono crate documentation](https://docs.rs/chrono/latest/chrono/format/strftime/).
	Use `cast(series, :datetime)` if you prefer the format to be inferred (if possible).

[anthony-khong]

I've incorporated the suggestion. Thank you!

[josevalim]

LGTM but I will let @philss merge with another pass. :)

[philss]

LGTM!

Maybe we could add support for the :category dtype in the future. But for now it LGTM.

[philss]

@anthony-khong thank you! 💜

[anthony-khong]

Thanks for the approval @philss!

Maybe we could add support for the :category dtype in the future. But for now it LGTM.

I'm trying to take a stab at this. Reading the Polars source code, I don't see a direct way to convert the categorical type to datetime. Should we do:

1. Cast the categorical series to Utf8 first, then do strptime; or
2. Downcast to CategoricalChunked, do iter_str( ... ) and try to emulate as_datetime?

I think option 1 will guarantee a more consistent behaviour as as_datetime is quite complex, but I'm not sure if there's a downside to casting.

[philss]

Hey @anthony-khong

I think the option 1. is easier to implement, and could be our first version. But I think we could do something in the lines of option 2., but instead of inter_str(...), we could convert the CategoricalChunked into a series - there is a into_series() method -, apply the transformation, and then work with the indexes of both the logical series of the categorical series, and the indexes of this new series to create the final series. It's a similar dance that we do in the encoding of categorical series to a list, in the encoding file, but simplified:

https://github.com/elixir-nx/explorer/blob/6671ae3cc323d4c1bac2a944cbb991284ad86446/native/explorer/src/encoding.rs#L349

I didn't test though. It may be slower then doing 1., but I think it is more memory efficient. WDYT?

[josevalim]

If you have strings and they are datetimes, then you want to convert them to datetimes before you convert them to categories. So I am fine with not supporting it.

[anthony-khong]

@philss, yes that makes a lot of sense - definitely more memory efficient. That's really cool, thanks for showing me that!

My concern is applying this transformation - it looks quite complex and there are changes between 0.29 and 0.30.

In any case, in light of José's comment, perhaps I should leave it be for now 😅

[philss]

@anthony-khong yeah, I agree that we should leave for now :D