test(python): add parametric tests for groupby_dynamic

[MarcoGorelli]

Thanks @honno for helping write these - any more suggestions?

This has already helped uncover:

• 4 bugs in pandas: BUG: Unnecessary NonExistentTimeError when resampling weekly pandas-dev/pandas#53666, BUG: resampling empty series loses time zone from dtype pandas-dev/pandas#53664, BUG: Timestamp origin takes no effect in resample for 'MS' frequency pandas-dev/pandas#53662, BUG: resample with freq W-Mon and first data point starts on Monday doesn't respect closed pandas-dev/pandas#53612
• 1 bug in polars (groupby_dynamic doesn't truncate label when the number of periods is greater than 1 #9333) (now fixed! 🚀 )
• 1 bug converting polars to pandas (Wrong time stamp when converting to pyarrow for dates after 2038 #9315)

The two tests presented here take about 20s to execute on my very mediocre 4-core travelling laptop the 3 tests here take about 15s on my laptop

[honno]

Mind I'm pretty ignorant on the problem domain here, but it looks good to me!

On purposely restricting strategies so they don't trigger known bugs—I think it makes sense here, given the sheer number of permutations. Ideally one would have the strategies cover all supposedly-valid input and slapping a xfail when needed, but that gets annoying fast when you have such a complex domain with multiple known bugs.

I did a diff on the two tests and it does look annoying enough to refactor, not that refactoring does much good when things have only been repeated once. Just to say if there was a way to have these tests neatly refactored (i.e. a big _test_against_pandas function called in both tests, or a parametrized arg like test_weekly: bool in a single test), defo play around with it if you haven't already.

[honno]

Could you instead construct the strategy for timezone to just be the available timezones? e.g. timezone=st.sampled_from(zoneinfo.available_timezones())

[alexander-beedie]

At the moment these tests are very bespoke, in that they don't use any of the existing polars hypothesis primitives/strategies; It would be nice if we could pull out the datetime (with timezone) generating strategy as a customisable create_datetime_strategy helper in the same vein as the existing create_list_strategy (timezones have been on my radar for a while, but I hadn't got round to it yet - currently datetime generation just varies by time_unit).

Either I can have a crack at that, or I can leave it to one of you and then review? Either option is fine by me... ;)

[MarcoGorelli]

Thanks @honno ! Indeed, it was possible to simplify a bit, thanks for suggesting that

And thanks @alexander-beedie for taking a look! Which part are you suggesting to make a strategy for - creating a polars Series with Datetime dtype?

[alexander-beedie]

And thanks @alexander-beedie for taking a look! Which part are you suggesting to make a strategy for - creating a polars Series with Datetime dtype?

We can already do that easily enough, just not with customisable timezones - would be nice to improve that so we don't have to go bespoke once a timezone comes into play :)

[honno]

Okay to reconstruct the current way of generating the original df example, it goes something like

@st.composite
def timey_wimey_dataframes(draw: st.DrawFn) -> pl.DataFrame:
    datetimes = draw(
        st.lists(
            st.datetimes(
                min_value=dt.datetime(1980, 1, 1),
                # Can't currently go beyond 2038, see
                # https://github.com/pola-rs/polars/issues/9315
                max_value=dt.datetime(2038, 1, 1),
            ),
            min_size=1,
        )
    )
    timezone = draw(st.sampled_from(list(zoneinfo.available_timezones())))
    
    nrows = len(datetimes)
    values = draw(
        st.lists(st.floats(10, 20), min_size=nrows, max_size=nrows), label="values"
    )

    try:
        df = (
            pl.DataFrame({"ts": datetimes, "values": values})
            .sort("ts")
            .with_columns(
                pl.col("ts").dt.replace_time_zone("UTC").dt.convert_time_zone(timezone)
            )
        )
    except pl.exceptions.ComputeError as exp:
        assert "unable to parse time zone" in str(exp)  # sanity check
        reject()

    return df

strat = timey_wimey_dataframes()

Using polars.testing I suppose you do something like

strat = pl.testing.dataframes(
    cols=[
        pl.testing.column(
            name="ts",
            dtype=pl.Datetime(),
            strategy=st.datetimes(
                min_value=dt.datetime(1980, 1, 1),
                max_value=dt.datetime(2038, 1, 1),
                timezones=st.sampled_from(list(zoneinfo.available_timezones())),
            ),
        ),
        pl.testing.column(name="values", dtype=pl.Float64(), strategy=st.floats(-10, 10)),
    ]
).map(lambda df: df.sort("ts"))

? Hopefully this is a good start @MarcoGorelli

In any case, I used the timezones strategy directly in st.datetimes(), but I wonder if that causes problems?

(I haven't actually tried this—I suppose polars.testing doesn't show up from like PyPI builds, so will have to try this later when I set up polars for development)

[MarcoGorelli]

cool, I've given this a go - thanks @honno ! any further suggestions?

[honno]

I wonder if testing.parametric.series can be utilised here?

Haven't tried this myself yet, but something like

Suggested change

	values = pl.Series(
	values = data.draw(pl.testing.parametric.series(strategy=st.floats(10, 20)), label="values")

(Incase you missed it, you can annotate draws with labels, which can make debugging easier for more complicated tests. No biggie tho.)

[alexander-beedie]

@honno: Interesting; do you think it would be worth putting any labels into the underlying primitives themselves?

[honno]

@honno: Interesting; do you think it would be worth putting any labels into the underlying primitives themselves?

I think I'm correct in saying that there's no real point putting labels inside of @st.composite-functions, as Hypothesis would only report the overall thing been drawn at the test level, i.e. test_foo(data): data.draw(dataframes(...), label="only the final df is reported"). Labeling seems more useful anyway when a bespoke thing is for a test method.

Also, it's really cool that these primitives exist!

[alexander-beedie]

Looking at the tests I think a few more native params might be useful too; some generic min_value & max_value args, for example... Don't hesitate to request anything you think would be a time-saver / useful :)

[MarcoGorelli]

not sure I see what you mean exactly, could you clarify what you're suggesting I change please?

[alexander-beedie]

Oh! That’s more of a note to myself than you, sorry… Just thinking I can extend the primitives a bit to further simplify common cases 🤣

[honno]

On the usage of Hypothesis, everything LGTM!

Already noted to Marco before that I would name strategy-factories (e.g. @st.composite-decorated functions that return st.SearchStrategy objects) be named "thing-it-generates" (preferably in plural), like dataframes() or series(). But I see the existing convention in polars.testing.parametric.strategies is to name these with the strategy_ prefix, so no biggie.

[MarcoGorelli]

thanks!

@alexander-beedie any objections to getting this in? it would add 20 seconds or so to CI, but alongside the unit tests (which this is absolutely not a replacement for) would increase confidence in future refactors of groupby_dynamic a fair bit

EDIT: marking as draft for now, gonna see if I can reduce the runtime whilst keeping the extra safety it provides

[MarcoGorelli]

closing for now to clear the queue, will get back to this when I get a chance