MAINT - changing TableVectorizer tests so that they use df_module

[gabrielapgomezji]

Modifying tests from table vectorizer to add df_module.
Changed a part, but there are others that might need a bit more change as test_handle_unknown_category

[rcap107]

Addressing #1441

[gabrielapgomezji]

For the moment I am changing the dataframe inside the test but later I will update the _get_clean_dataframe function

[rcap107]

This PR was complicated by the fact that some of the tests are checking for pandas-specific behaviors and conditions.

Some tests must be skipped with min-optional-reqs because polars support was added in scikit-learn 1.4, so in some specific cases tests could not pass when run using polars.

The main changes revolved around modifying the functions _get_clean_dataframe etc so that they'd use df_module, rather than returning directly a pandas dataframe. As a result, fixtures had to defined in a different way (see X_tuple_fixture).

Some other changes had to be made to get around how polars and pandas have different indexing and dtype representations.

At this point, most of the tests (those that are not being skipped, at least) fully support both pandas and polars. However, there are some tests that may need some refinement.

Given that this isn't a user-facing issue, and in the interest of avoiding conflicts, I think we should merge this and leave further improvements to future PRs.

[GaelVaroquaux]

I would have found it markedly more readable to use "@pytest.mark.parametrize" (https://docs.pytest.org/en/stable/how-to/parametrize.html#parametrize-basics) instead of the fixture.

Is there a reason that you did not consider this option?

[rcap107]

I don't think parametrize plays well with the fact we need to pass df_module to the functions

[GaelVaroquaux]

I had a comment about the fixture, which I find less readable than a parametrize.

However, I realize that we are already using super massively fixtures in skrub test, so it's consistent with our fixture usage. Hence, I'm happy to take push back on my comment.

[GaelVaroquaux]

Same thing, I would much rather prefer a "@pytest.mark.parametrize", which is more explicit, than the fixture

[rcap107]

Is this what you'd expect with parametrize? I find this far less readable than the fixture to be honest.

@pytest.mark.parametrize(
    "X",
    [
        pytest.param(_get_clean_dataframe, id="clean_dataframe"),
        pytest.param(lambda df_module: _get_dirty_dataframe(df_module, categorical_dtype="object"), id="dirty_dataframe_object"),
        pytest.param(lambda df_module: _get_dirty_dataframe(df_module, categorical_dtype="category"), id="dirty_dataframe_category"),
        pytest.param(_get_mixed_types_dataframe, id="mixed_types_dataframe"),
        pytest.param(lambda _: _get_mixed_types_array(), id="mixed_types_array"),
    ],
)

Unless there is a way of accessing df_module without the lambda functions, I think that having parametrize makes harder to understand

[GaelVaroquaux]

Yes, this is what I was expecting.

I find this more readable because it introduce less conceptual things to understand and more locality in the code: the values are defined explicitly close where they are used. The fixture defines things very implicitly and far.

[rcap107]

I had a comment about the fixture, which I find less readable than a parametrize.

However, I realize that we are already using super massively fixtures in skrub test, so it's consistent with our fixture usage. Hence, I'm happy to take push back on my comment.

Part of why we used fixtures was because they're already very heavily used in the tests, however I think they're also more readable in this situation, where we need to pass df_module to the functions in order to generate the different variants of the datasets.

[GaelVaroquaux]

LGTM

Thanks a lot @gabrielapgomezji and @rcap107 !!