Bug in to_frame.origin_as_datetime argument

[kennethshsu]

Will try to create a new ticket so we can better keep track for patch notes going forward.

Is there a standardized warning message to use? I was thinking
Deprecation warning: in an upcoming version of the package, origin_as_datetime will be defaulted to True in to_frame(...), use origin_as_datetime=False to preserve current setting.

[jbogaardt]

Here is an active deprecation notice in the library currently -

chainladder-python/chainladder/__init__.py

Lines 28 to 30 in 3398979

	@deprecated("Use chainladder.options.set_option('ARRAY_BACKEND', value) instead.")
	def array_backend(array_backend="numpy"):
	options.set_option('ARRAY_BACKEND', array_backend)

I haven't given much thought to good messaging though. I'm sure whatever you choose will work just fine.

[kennethshsu]

@jbogaardt do you know if origin_as_datetime actually works properly?

import pandas as pd
import chainladder as cl

quarterly = cl.load_sample("quarterly")
quarterly["paid"].to_frame(origin_as_datetime=True).index == quarterly["paid"].to_frame(
    origin_as_datetime=False
).index

[jbogaardt]

Hah, I guess it did not. No sense in having a deprecation notice on a bug.

[kennethshsu]

lol... Hmm, what should we do here? Let's close this ticket and open a new one?

[jbogaardt]

Repurposed this issue as a bug.

[kennethshsu]

I think I was able to fix the bug, but now the question is I actually don't know if origin_as_datetime is preferred and should therefore be the default. PeriodIndex and DatetimeIndex work almost the same. I think there's very little benefit with having one over the other.

I actually think it's more useful to have origin_as_string or something that is immediately usable without too much fiddling with .strftime(...). Most of the time when to_frame(...) is called is when we are ready to export or graph, so having something as string is easier to work with. Thoughts?

[jbogaardt]

I agree, since this was a bug, let's just make datetime the default. I like datetimes because they can be resampleed in pandas to a higher grain. For plotting, almost all libraries, like plotly and bokeh, support datetime (less so periods) so I do think datetimes have an advantage. Are you converting to strings for plotting? I would think this is unnecessary. Strings are okay, but support a much narrower set of use-cases.

[kennethshsu]

Wait, why would we want to make datetime the default? Or do you mean the default going forward (going through the usual deprecation cycle)?

I've been working around PeriodIndex because like you said, it's not super graphing friendly. I think I need to better learn how to work with datetime directly (plotting, data wrangling, etc) without converting to strings.

[jbogaardt]

Yah, I think making datetime the default in to_frame is reasonable. When you have a multi-dimensional triangle, like clrd, it already defaults to datetime. The plotting benefits of datetime over period are further reinforce it as a better choice in to_frame. I don't think we should move away from period in the triangle, only upon moving to_frame.

[kennethshsu]

There's a PR now, but I am still using origin_as_datetime = False as the default, there's a deprecation warning though.

I think it'll be safer to leave it at that for now. We can move over to True in the next version?

[jbogaardt]

Thanks, this is great! I've played around with it and it mostly does what I would expect. The one area it doesn't is on multi-dimensional triangles. I think it always coerces to a datetime regardless. Consider this unit test:

import chainladder as cl
clrd = cl.load_sample('clrd')

def test_origin_as_datetime_arg(clrd):
    from pandas.api.types import is_datetime64_any_dtype
    assert is_datetime64_any_dtype(clrd).to_frame(origin_as_datetime=True)['origin'])
    assert not is_datetime64_any_dtype(clrd).to_frame(origin_as_datetime=False)['origin'])

test_origin_as_datetime_arg(clrd)

[kennethshsu]

Do you have an extra ) right after clrd in each of your assert statements?

Anyways, I didn't get an error with this.

clrd = cl.load_sample("clrd")

assert is_datetime64_any_dtype(clrd.to_frame(origin_as_datetime=True)["origin"])
assert not is_datetime64_any_dtype(clrd.to_frame(origin_as_datetime=False)["origin"])

With this:

clrd.to_frame(origin_as_datetime=False)["origin"]

I got Name: origin, Length: 33089, dtype: period[A-DEC].

[jbogaardt]

yes, thanks. At least I wrote it correctly in the unit test.