Relax check_X_y for DecayEstimator

[FBruzzesi]

Description

Relaxes the check for X in DecayEstimator by delegating the actual checks to the given estimator. This fixes #573 and allows estimators that do not need X features.
Remark, as discussed in the issue, it may be worth it to completely remove the check.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the style guidelines (flake8)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (also to the readme.md)
• I have added tests that prove my fix is effective or that my feature works
• I have added tests to check whether the new feature adheres to the sklearn convention
• New and existing unit tests pass locally with my changes

[koaning]

I'm also OK with removing the entire check. Mainly because the internal method should already be doing that and this way we'll need to make fewer assumptions.

[FBruzzesi]

I'm also OK with removing the entire check. Mainly because the internal method should already be doing that and this way we'll need to make fewer assumptions.

To be completely pedantic, the check_X_y function call checks and converts X and y to arrays. This allows to then access .shape attribute to know how many samples are there and compute the decay.

Now both approaches feel wrong and with shortcomings:

• Using a check, it may lead to errors or different results from the check in the estimator (e.g. if the estimator allows for multioutput while the check we are doing doesn't)
• Not having a check doesn't allow to access .shape or len in the general case (I noticed this by running unit tests) and I would be unsure how to proceed.

Opinions?

[koaning]

Not having a check doesn't allow to access .shape or len in the general case (I noticed this by running unit tests) and I would be unsure how to proceed.

Ah, I forgot about that. That's a fair point. I guess the simplest solution is to offer a flag to the end-user and to defer the responsibility of applying the check to them? That said, I think most of the time X will be .shape-able or len()-able so the default could be True. Or am I missing something?

[FBruzzesi]

I guess the simplest solution is to offer a flag to the end-user and to defer the responsibility of applying the check to them?

That's reasonable!

That said, I think most of the time X will be .shape-able or len()-able so the default could be True

With such default the scikit-learn tests would fail. I will add a big warning in the docstring 😁

[koaning]

With such default the scikit-learn tests would fail. I will add a big warning in the docstring 😁

There are a lot of components in this library that fail the standard checks. That's ok. It's not ideal, but given how "non-standard" some of these ideas are ... it's fine to turn a test off sometimes. Adding a warning to the docstring is fair though!

[koaning]

This is a bit of a nitpick.

Suggested change

	self, model, decay: float = 0.999, decay_func="exponential", check_X_y=False
	self, model, decay: float = 0.999, decay_func="exponential", check_input=False

My reasoning here is that some estimators may only have a check_X. This is the case when the component is a scikit-learn transformer. But it'd be nice to have a consistent name for checking the internal thing. So maybe this namechange?

Once that change is in/discussed I think we can hit merge :)

[FBruzzesi]

That's very reasonable 👌 let me adjust that!

[koaning]

Left one comment related to the name of a variable

[koaning]

LGTM!