feat: 44 sklearn compatible interface (with fkiraly)

[astrogilda]

Implements #44

---

Update by @fkiraly:

• merges configs and classes for public interface - configs are still used internally, but not exposed to hte user
• makes all kwargs parameters explicit
• completes docstrings with a description of all valid kwargs

[fkiraly]

Tests are failing because you always need to import the config classes in base_bootstrap.py, but you only import them for type checking. (the imports are under an "if" condition)

Are you running tests locally?

[astrogilda]

Huh I can't remember when or why I put those impirts under an if condition. I can remove them and recommit. Nah this time I did not run them locally, relied on github actions to test them.

…

On Tue, Feb 6, 2024, 8:14 PM Franz Király ***@***.***> wrote: Tests are failing because you *always* need to import the config classes in base_bootstrap.py, but you only import them for type checking. (the imports are under an "if" condition) Are you running tests locally? — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTOOHU2PKQSP6QZJBGCFZ3YSLIP7AVCNFSM6AAAAABC44QJD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZRGA3TANRRGI> . You are receiving this because you were assigned.Message ID: ***@***.***>

[fkiraly]

well, they tested them alright

anyway, more tests are passing now, and the properties no longer seem a problem!

So we are now properly running the tests!
Some findings:

• BaseTimeSeriesBootstrap should super.__init__ (and every class with an __init__ should)

[fkiraly]

question: do we want to allow __init__ kwargs? If yes, we have to override the standard tests. It is safer to remove kwargs in class constructors, but it will probably not cause too many problems if we leave them.

on the other hand, adding args could keep downwards compatibility:

if isinstance(args[0], TimeSeriesBootstrapConfig):
    self.set_params(**args[0].get_params())

but this is maybe complicating it too much?

[astrogilda]

I am leaning more towards keeping kwargs. How exactly would tests need to be overridden? Best, Sankalp Gilda, Ph.D.

…

On Wed, Feb 7, 2024 at 3:42 PM Franz Király ***@***.***> wrote: question: do we want to allow __init__ kwargs? If yes, we have to override the standard tests. It is safer to remove kwargs in class constructors, but it will probably not cause too many problems if we leave them. — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTOOHWUD7LCQ7EI4XSRSEDYSPRLNAVCNFSM6AAAAABC44QJD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZSHAZTIOBTG4> . You are receiving this because you were assigned.Message ID: ***@***.***>

[fkiraly]

overriding test_constructor by inheritance in TestAllBootstraps.

It's a larger test, so I'd feel a bit uneasy about it, unless we catch only that exception specifically.

[astrogilda]

Okay, I can take a look.

…

On Wed, Feb 7, 2024, 3:54 PM Franz Király ***@***.***> wrote: overriding test_constructor by inheritance in TestAllBootstraps — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTOOHVVVI3TPCCDMKCUQ3DYSPSZXAVCNFSM6AAAAABC44QJD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZSHA3DONJUGU> . You are receiving this because you were assigned.Message ID: ***@***.***>

[fkiraly]

Okay, I can take a look.

I'd say, let's keep with the plan for now?

[astrogilda]

Wait, what plan? Sorry I'm confused. I thought you said if one wants to retain kwargs then the tests need to be changed?

…

On Wed, Feb 7, 2024, 5:05 PM Franz Király ***@***.***> wrote: Okay, I can take a look. I'd say, let's keep with the plan for now? — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTOOHXK22XRI2FAZHXR573YSP3EHAVCNFSM6AAAAABC44QJD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTGAYTKNBTGQ> . You are receiving this because you were assigned.Message ID: ***@***.***>

[fkiraly]

I thought you said if one wants to
retain kwargs then the tests need to be changed?

Yes, but that wasn't your ticket - so to avoid clashing PR, I'm happy to make the change in tests (if we decide so), and you continue working on the API change?
The new tests ought to change in the new test PR anyway.

My confusion comes from you saying

Okay, I can take a look.

which seems like you want to work on the test classes?

[astrogilda]

aaaah. Yes, please go ahead. I won't touch that issue then. Best, Sankalp Gilda, Ph.D.

…

On Wed, Feb 7, 2024 at 5:47 PM Franz Király ***@***.***> wrote: I thought you said if one wants to retain kwargs then the tests need to be changed? Yes, but that wasn't your ticket - so to avoid clashing PR, I'm happy to make the change (if we decide so), and you continue working on the API change? — Reply to this email directly, view it on GitHub <#66 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFTOOHXLNKQ6FF2OEZKRDXTYSQABHAVCNFSM6AAAAABC44QJD2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTGA3DCNZWHA> . You are receiving this because you were assigned.Message ID: ***@***.***>

[fkiraly]

really strange sporadic errors on specific os-version combinations. The association with the os-version combination seems stable.

I would suspect this an actual error rather than one introduced in the interface rework, but of course it could be the latter to.

[fkiraly]

MRE for the failure:

import numpy as np
from tsbootstrap.block_bootstrap import BaseBlockBootstrap

bootstrap_type = "stationary"
n_bootstraps=1
rng=None
X=np.array([[0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.],
       [0., 0.]])

block_length = np.random.randint(1, int(0.8 * X.shape[0]) - 1)
bootstrap = BaseBlockBootstrap(
    bootstrap_type=bootstrap_type,
    block_length=block_length,
    n_bootstraps=n_bootstraps,
    rng=rng,
)

list(bootstrap.bootstrap(X))

[fkiraly]

Is this maybe a masked bug on main? The code on main does not seem to enter BlockGenerator, while the code on this branch does.