Index constructor doesn't validate kwargs

[TomAugspurger]

In [4]: pd.Index([1, 2], abc=1)                                                                                                                                                                                                                                                                                                                                           
Out[4]: Int64Index([1, 2], dtype='int64')

This should raise, since abc isn't a valid keyword.

[adriesse]

Is the goal in pandas, to raise an exception whenever a non-existing kwarg is used?

[TomAugspurger]

Yes.

[bartkim0426]

Hi, is there someone working for it? If not, can I make PR for this?

[TomAugspurger]

Sure. Here's how I would do it.

Start in Index.__new__ at

pandas/pandas/core/indexes/base.py

Line 259 in e26fa2b

def __new__(

and look at all the places we pass through **kwargs. This will be index subclasses like Categorical, IntervalIndex, PeriodIndex, etc. Find the set of arguments that those accept. If any of them accept **kwargs, then we should start by narrowing down the valid set of kwargs first (in its own PR, one per subclass).

@jreback @jorisvandenbossche two API questions:

1. What do we do with subclass-specific kwargs like closed for IntervalIndex? Do we add them to the Index class? Do we have a dedicated subclass_kwargs argument that's a dict of kwargs for the subclass (that's not a good name. Maybe not a good idea). Or do we keep kwargs, and just raise when an invalid one is passed?
2. What do we do now with invalid kwargs? Raise, or warn that they'll raise in the future?

[jorisvandenbossche]

What do we do with subclass-specific kwargs like closed for IntervalIndex?

They are currently passed through?

[TomAugspurger]

Yes. The method depends a bit on the subclass. I glanced briefly, but CategoricalIndex gets **kwargs. IntervalIndex gets kwargs.get("closed", None). So they're either all passed through, or they get specific ones.

[jorisvandenbossche]

We could also deprecate that? if you want the specific keywords, you can use the specific class constructor?

What do we do now with invalid kwargs? Raise, or warn that they'll raise in the future?

I would lean towards first a warning. Users could have unintentionally done this (eg typo).

[bartkim0426]

Thanks for detail explanation. I'll make a work for this - raise a error or just add warning.

[bartkim0426]

@TomAugspurger I try to modify code - make a warning for invalid kwargs.
Then we have to specify a list for correct kwargs - and should check every kwargs that they're in the correct list. Is this what you're going to change? I can't find python code that warns with invalid kwargs. Please give detail thinking about it. Thanks!

[TomAugspurger]

Here's how I might do this:

1. Ensure that every Index subclass does not take **kwargs.
2. Make the named arguments in Index.__new__ just the arguments that are accepted by all subclasses (maybe treat tupleize_cols specially, since it's not meant to be passed through)
3. Always pass **kwargs through to the subclasses constructor. This validates **kwargs for us when we're returning a subclass
4. When returning an object-dtype Index (the last case in the if / elif chain), validate that **kwargs is just the expected arguments. I think just

expected_kwargs = {'data', 'dtype', 'copy', 'name', 'tuplize_cols'}
extra = set(kwargs) - expected_kwargs
if set(kwargs) - expected_kwargs:
    warnings.warn(...)
    return Index(subarr, dtype=dtype, copy=copy, name=name)

Does that make sense?

[bartkim0426]

@TomAugspurger
Thanks for the explanation and sorry for late response.

First, I checked that every subclass doesn't take **kwargs.
(NumericIndex, MultiIndex, CategoricalIndex, DatetimeIndex, TimedeltaIndex, PeriodIndex, RangeIndex, Int64Index, NumericIndex)

I was confused about always pass **kwargs through to the subclasses constructor.
For subclasses - if there are invalid args, TypeError occurred so I think we don't have to validate the subclasses.

Then is it enough to add validate code below __new__ of Index? I think

def Index(...)
    ...
    def __new__(
        cls,
        data=None,
        dtype=None,
        copy=False,
        name=None,
        fastpath=None,
        tupleize_cols=True,
        **kwargs
    ):
        expected_kwargs = {'data', 'dtype', 'copy', 'name', 'tuplize_cols'}
        if set(kwargs) - expected_kwargs:
            warnings.warn(...)
        ... 

Then users can see warnings while calling Index with invalid **kwargs, and faces error while calling index's subclasses. Please tell me if I misunderstand something. Thanks again!

[TomAugspurger]

I don't think the maintainers have agreed on a desired behavior yet, sorry.

In another issue (can't find it now) @WillAyd expressed displeasure with accepting both name and names (for MultiIndex). So perhaps he agrees with @jorisvandenbossche's suggestion to deprecate passing through subclass-specific kwargs.

cc @jreback if you have thoughts on not passing through subclass-specific kwargs.