Refactor transformed estimators about sknnr HOT 6 CLOSED

Should we stick with transform_ for the attribute, or do you think we should go with transformer_?

Ooo, maybe transformer_ is better to pair with the class names (CCATransformer, MahalanobisTransformer, etc.). Maybe we've gone back and forth on this one 😉

from sknnr.

@aazuspan, I like it! I especially like the prevention of instantiation by code rather than by convention and, as you say, it gets rid of some duplicated code as well. A couple of quick questions/comments just to make sure I get it:

• GNN and MSN would be, for example, GNNRegressor(YFitMixin, TransformedKNeighborsRegressor), correct? Just wanted to verify that YFitMixin.fit gets called before TransformedKNeighborsRegressor.fit ...
• I don't know if this was intentional, but _set_fit_transform could be confused with the more generic fit_transform method defined in each transformer, which fits/transforms in a single step. But I think the intention of _set_fit_transform is really just to set the transform? I actually wonder if it's even necessary to have that method - perhaps this instead? Is there an advantage to this that I'm overlooking?

class TransformedKNeighborsRegressor(RawKNNRegressor, ABC):
    ...

    @abstractmethod
    def _get_transform(self) -> TransformerMixin:
        """Return the transformer to use for fitting. Must be implemented by subclasses."""
        ...

    def fit(self, X, y):
        """Fit using transformed feature data."""
        self._validate_data(X, y, force_all_finite=True, multi_output=True)        
        self._set_dataframe_index_in(X)
        self.transform_ = self._get_transform().fit(X, y)

        X_transformed = self.transform_.transform(X)
        return super().fit(X_transformed, y)

from sknnr.

GNN and MSN would be, for example, GNNRegressor(YFitMixin, TransformedKNeighborsRegressor), correct?

Bingo!

I don't know if this was intentional, but _set_fit_transform could be confused with the more generic fit_transform method defined in each transformer, which fits/transforms in a single step. But I think the intention of _set_fit_transform is really just to set the transform?

Yes, great point! That was unintentional - we should definitely choose a better name. Is _set_fitted_transform still too similar? Or _set_fitted_transformer (probably combined with _get_transformer)?

I actually wonder if it's even necessary to have that method - perhaps this instead? Is there an advantage to this that I'm overlooking?

Separating out the transformer fitting is a workaround to an inheritance problem that arises when YFitMixin.fit calls super().fit, which calls TransformedKNeighborsRegressor.fit and refits the transformer with the wrong y data. I also played around with 1) adding a reset=True argument to TransformedKNeighborsRegressor.fit to avoid resetting the transformer if called intentionally, and 2) having TransformedKNeighborsRegressor.fit accept **kwargs and fit the transformer with y_fit if present. Those generally worked, but I wanted to keep as much of the y_fit logic out of TransformedKNeighborsRegressor as possible, and having a separate fitting method allowed that.

It's 100% possible there's a better workaround that I didn't think of though!

from sknnr.

Yes, great point! That was unintentional - we should definitely choose a better name. Is _set_fitted_transform still too similar? Or _set_fitted_transformer (probably combined with _get_transformer)?

I guess I have a slight preference for the latter, but either is fine by me.

Separating out the transformer fitting is a workaround to an inheritance problem that arises when YFitMixin.fit calls super().fit, which calls TransformedKNeighborsRegressor.fit and refits the transformer with the wrong y data. I also played around with 1) adding a reset=True argument to TransformedKNeighborsRegressor.fit to avoid resetting the transformer if called intentionally, and 2) having TransformedKNeighborsRegressor.fit accept **kwargs and fit the transformer with y_fit if present. Those generally worked, but I wanted to keep as much of the y_fit logic out of TransformedKNeighborsRegressor as possible, and having a separate fitting method allowed that.

Oof, let's avoid that complexity! Having a separate _set_fitted_transform or _set_fitted_transformer method sounds like a preferable solution.

from sknnr.

I guess I have a slight preference for the latter, but either is fine by me.

Agreed! Should we stick with transform_ for the attribute, or do you think we should go with transformer_?

Oof, let's avoid that complexity!

Sounds like a plan! I already worked most of this up to make sure it was possible, so I'll make a PR shortly.

from sknnr.

Resolved by #52

from sknnr.