In TaggedTable override methods of Table that return a Table

[lars-reimann]

Is your feature request related to a problem?

Now that TaggedTable is a subclass of Table, it also inherits some methods that return a new Table. However, currently those methods lose the tags, since a Table simply doesn't know about them. Methods that remove tagged Columns must
throw an IllegalSchemaModificationException.

Desired solution

Override the methods and return a new TaggedTable instead.

Possible alternatives (optional)

No response

Screenshots (optional)

No response

Additional Context (optional)

No response

[lars-reimann]

Blocked by #190.

[zzril]

Converning the InvalidSchemaModificationError that shall be thrown when attempting to remove the target column from a TaggedTable: Should this exception also be documented in the docstring of the parent class's implementation of the corresponding method?

I guess in a proper statically-typed language like Java, this would be the way to go (subclass can only throw fewer or more concrete exceptions than the parent class), but I'm not sure how this is typically done in Python.

[lars-reimann]

Only put it in the docstring of the subclass.

[zzril]

Regarding the transform (and possibly inverse_transform) methods of TableTransformer: Shall these also maintain tagging?

That could not be achieved by overriding. Instead, one could add a check in the fit method to see if the table is tagged and if so, save the corresponding column name in the transformer. Then restore tagging at the end of the transform methods.
Might be a bit trickier with those transformers that can also change / remove columns (e.g. the OneHotEncoder).

Might be worth a new issue, though.

[lars-reimann]

No, only the transform and inverse_transform methods of TaggedTable should have that property.

[zzril]

Only put it in the docstring of the subclass.

Turns out this behaviour of letting the subclass throw more exceptions is problematic.

Some of the tests (e.g. test_should_raise_if_dataset_misses_features in tests/safeds/ml/classical/regression/test_regressor.py mistakenly call remove_columns on a TaggedTable when they really should be using a normal Table.

Now while we could just go ahead and change the tests to use Tables instead, the fact that we use inheritance for the Table / TaggedTable would indicate that this should not really be a problem. Replacing a usage of a Table with a subclass, e.g. TaggedTable, should - in theory - not break anything. And this is also what users of the library may expect.

So, we either need to change the signature/docstring of the method in the parent class to make clear that the method can always throw the exception, so the caller must be prepared to handle it.
Or we could have remove_columns return a Table even in the child class. In this case, we could additionally provide a new method remove_feature_columns in the subclass that returns a TaggedTable and possibly throws the exception.

@lars-reimann

[guenterk]

For simplicity and uniformity, implement the first solution proposed by @zzril : "Change the signature/docstring of the method in the parent class to make clear that the method can always throw the exception, so the caller must be prepared to handle it."

For uniformity, apply the same schema if similar cases occur in the future (allways make sure that the supertype's contract is general enought).

[zzril]

Alright.

Some of the ML tests currently use remove_columns to extract the feature columns out of a TaggedTable, though, which will not be possible after changing it to always return a TaggedTable or throw.

So, we will have to implement a get_features_as_table or remove_target_column method anyway, for these tests to use.

Example from test_classifier.py:

    def test_should_include_complete_input_table(self, classifier: Classifier, valid_data: TaggedTable) -> None:
        fitted_regressor = classifier.fit(valid_data)
        prediction = fitted_regressor.predict(valid_data.remove_columns(["target"]))
        assert prediction.remove_columns(["target"]) == valid_data.remove_columns(["target"])

[guenterk]

End of day status report by @zzril: Implemented the above solution, except for the "keep_only" method of TaggedTable, which is heavily used throughout tests and the implementation of the TaggedTable class itself (e.g. in the constructor) in a way where returning a TaggedTable breaks existing code. Wee need to discuss these cases and determine how to treat them before we can complete this issue.

[lars-reimann]

🎉 This issue has been resolved in version 0.15.0 🎉

The release is available on:

• v0.15.0
• GitHub release

Your semantic-release bot 📦🚀