Argument validation and documentation of exceptions in _from_table in _tagged_table.py

[zzril]

The static _from_table method in _tagged_table.py partly validates its arguments like this:

        # If no feature names are specified, use all columns except the target column
        if feature_names is None:
            feature_names = table.column_names
            if target_name in feature_names:
                feature_names.remove(target_name)

        # Validate inputs
        if target_name in feature_names:
            raise ValueError(f"Column '{target_name}' cannot be both feature and target.")
        if len(feature_names) == 0:
            raise ValueError("At least one feature column must be specified.")

One obvious case of "bad input" is not recognized here: target_name may not match any of table's columns at all.
In this case, an UnknownColumnNameError would be silently thrown a few lines later:

        result._target = result.get_column(target_name)

This possible exception is also not documented in the docstring. (It is, however, tested for in one of the tests.)

Is there a specific reason to implement it like this?

Possible solution

Test for target_name not in table.column_names at the beginning of the function and throw the appropriate exception when the condition is met.
Mention the exception in the docstring.

Can then also drop the unneccessary if target_name in feature_names: in line 84.

[lars-reimann]

Can then also drop the unneccessary if target_name in feature_names: in line 84.

That is another error: The column exists in the table but is used both as a feature and as the target.

That said, it indeed makes sense to raise the UnknownColumnNameError at the beginning of the _from_table to produce a shorter stacktrace.

[zzril]

Can then also drop the unneccessary if target_name in feature_names: in line 84.

That is another error: The column exists in the table but is used both as a feature and as the target.

I was talking about the second if here:

        # If no feature names are specified, use all columns except the target column
        if feature_names is None:
            feature_names = table.column_names
            if target_name in feature_names:
                feature_names.remove(target_name)

Once we have made sure that target_name is contained in table.column_names, the second if will always be true.

[lars-reimann]

Can then also drop the unneccessary if target_name in feature_names: in line 84.

That is another error: The column exists in the table but is used both as a feature and as the target.

I was talking about the second if here:

        # If no feature names are specified, use all columns except the target column
        if feature_names is None:
            feature_names = table.column_names
            if target_name in feature_names:
                feature_names.remove(target_name)

Once we have made sure that target_name is contained in table.column_names, the second if will always be true.

I get your point now.

[lars-reimann]

🎉 This issue has been resolved in version 0.14.0 🎉

The release is available on:

• v0.14.0
• GitHub release

Your semantic-release bot 📦🚀