same below

Once all modules are supported, should this be merge with the already-existing check_n_features_in check?

We also have another check that specifically checks for error when the number of features are inconsistent (I don't remember the name). Should this one be removed then? (If yes let's document it here and next to N_FEATURES_IN_AFTER_FIT_MODULES_TO_IGNORE please)

Yea we should merge it into check_n_features_in.

We also have another check that specifically checks for error when the number of features are inconsistent (I don't remember the name). Should this one be removed then?

I think you are referring to check_estimators_partial_fit_n_features. This new check adds two new requirements on top of check_estimators_partial_fit_n_features:

1. n_features_in_ is set during the first call to partial_fit.
2. More strict when it comes to the error message.

I updated the comment with the above message.

I was more referring to e.g. check_classifiers_train :

        msg = ("The classifier {} does not raise an error when the number of "
               "features in {} is different from the number of features in "
               "fit.")

but we can keep it as-is and remove later (or not, as long as it passes)

why is this needed?

I was using this to detect if parital_fit accepted classes. Turns out all classifiers has this in partial_fit, so I updated with just is_classifer.

Nit but I find that using hasattr is cleaner

this means that if y isn't passed the second time partial_fit is called, then the error won't be properly raised. Do we want that?

This kind of makes this inconvenient for calling in non-fit methods. For predict, we would set reset=False and not provide a y. This if statement almost assumes that _validate_data is only called in fit or partial_fit.

We spoke about it here: #18001

A solution would be to add a requires_y kwargs to _validate_data:

    def _validate_data(self, X, y=None, reset=True, requires_y='auto',
                       validate_separately=False, **check_params):

Where requires_y='auto' means using the tag and it can also be a bool.

IIRC our chat with @amueller a few months ago, the cleanest solution we had agreed on was to call _validate_data only in fit and in the first partial_fit, and to define another method for the rest (predict, transform, subsequent partial_fit, etc.). This might require changing _validate_data a little, which is fine since it's private

This means all fit calls must always explicitly pass y.

You mean that y must be explicitly passed to fit, or that y must be explicitly passed to _validate_data in fit?

Either way, I'm not sure I understand the reason.

I updated the PR to use the __NO_OP placeholder. This means that it will be the callers responsibly to call this with y to get the requires_y check:

self._validate_data(X, None)  # will check that `y` is consistent with `require_y` tag

self._validate_data(X)  # will ignore `requires_y` tag

The alternative is to recommend setting y (in _validate_data) everywhere in fit. In other words:

def fit(self, X, y=None):
    # note that `y` is set explicitly to `None`, because the docstring
    # says `y` is ignored
    X = self._validate_data(X, None)

This way the requires_y will always be checked with y.

This means that it will be the callers responsibly to call this with y to get the requires_y check

I think that's fine because the reason we introduced the tag was to give a decent error message when y should be passed (e.g. for supervized estimators), but the user left it to None. Without the tag, they would get a weird tuple unpacking error with X, y = _validate_data(X, y). In other words, the tag is only useful when we pass both X and y, so that's fine to ignore it when we just pass X.

Could you remove the use of the requires_y parameter? I still see it in a few places ;)

I think I am okay with delegating the responsibility of passing y to the _validate_data's caller:

• If the caller passes y, then _validate_data will validate y.
• If the caller does not pass y, then do nothing (in terms of y).

Edit: I wrote this before github updated my UI with your above message.

Could you remove the use of the requires_y parameter? I still see it in a few places ;)

All gone now!

lol I think this might be the only case where it would make sense to use optional instead of indicating default=.... But we can leave it as-is since it's still private.

If you really want to keep a module level constant as a default placeholder, I would rather use single underscore. It's enough to tell it's private. But I think the code would be clearer without defining a module level constant.

I would just set y="no_validation" instead of defining a module level constant as default placeholder with a rather implicit name.

That would also make the docstring easier to understand.