Add robustness to leading null values in series in 'ewma_by_time'.

[wbeardall]

Following up #71, I've added an ignore_nulls flag to 'ewma_by_time' with a default True value. The main design choices are as follows:

1. When the sequence starts with a NaN, all values in the EWMA should also be NaN, up until the first non-NaN index, which should return its own value.

2. A string of consecutive NaN values should all have the same output as proceeding non-NaN value, as no additional information has been added to the sequence.

[MarcoGorelli]

thanks for your pr

I think we need to distinguish nan with null here

[MarcoGorelli]

Nice one, thanks

Something to discuss is ignore_nulls, because it's different to Polars' own ewm_mean. There, ignore_nulls determines how the weights are calculated (based on absolute positions or not), but the output always contains a value:

In [18]: >>> import polars as pl
    ...: >>> s = pl.Series([1.1, 2.5, 2.6, 2.1, None, 5.1])
    ...: >>> s.fill_nan(None).ewm_mean(alpha=.1, ignore_nulls=True)
Out[18]:
shape: (6,)
Series: '' [f64]
[
        1.1
        1.836842
        2.11845
        2.113085
        2.113085
        2.842473
]

In [19]: >>> import polars as pl
    ...: >>> s = pl.Series([1.1, 2.5, 2.6, 2.1, None, 5.1])
    ...: >>> s.fill_nan(None).ewm_mean(alpha=.1, ignore_nulls=False)
Out[19]:
shape: (6,)
Series: '' [f64]
[
        1.1
        1.836842
        2.11845
        2.113085
        2.113085
        2.902107
]

Here, on the other hand, the weights are determined based on times, rather than based on positions. If we use the same argument name, then it risks being quite confusing

Maybe this is a chance to revisit the ewm_mean API in Polars itself...I think it was copied from pandas a few years ago, although now it probably makes sense to make some improvements on it

[wbeardall]

Yeah you're right about this inconsistency, I can see how this could be confusing... would it be better to call the kwarg fill_nulls or similar here? It's probably also worth expanding the documentation to explain comprehensively how this performs

[MarcoGorelli]

Does there need to be another kwarg for this? It's easy enough to just do .forward_fill, maybe the default should just be to preserve null values, and if anyone wants to forward-fill them, they can just do .forward_fill?

I've made an issue about this in Polars if you want to voice your opinion pola-rs/polars#15258

[wbeardall]

I think you're right here, making the null filling explicit using .forward_fill seems better both in terms of expression clarity, user control and interface simplicity.

[wbeardall]

That brings the question back to ewma_by_time though; should the default behaviour for ewma_by_time be that null values are not filled by default? This seems idiomatically correct, is trivial to implement, and removes the need for an additional kwarg here too. This would change my earlier PR back to simply making the current implementation robust to leading nulls.

[MarcoGorelli]

sounds good to me, thanks!

[wbeardall]

I've added the changes, and changed the heading to reflect this.

[MarcoGorelli]

Nice! minor comment but looks good

[MarcoGorelli]

outdated?

[wbeardall]

Good catch, thanks!

[MarcoGorelli]

isn't this going to panic if times[idx] is None? do we need to zip over both values and times here?

[wbeardall]

Yes, it will; I've fixed.

[MarcoGorelli]

thanks @wbeardall !

[MarcoGorelli]

@wbeardall would you be interested in trying to upstream this to Polars itself?

[wbeardall]

Yes, schedule allowing! What is your timeline looking like for upstreaming polars-xdt to the main codebase?

[MarcoGorelli]

There's no schedule, this was just created to allow to quickly make features available that it's not clear should be included in Polars itself.

There's appetite for upstreaming this feature, as well as business days, but the rest isn't as clear