[ENH] test for init signature conventions, simplified `get_n_bootstraps`

[fkiraly]

This PR further cleans up logic around the base interface contract, and adds tests.

• the contract tests now check, for all bootstraps, that:
  • n_bootstraps is the first parameter, with default 10
  • all parameters have defaults
• get_n_bootstraps now always returns self.n_bootstraps instead of reaching into the config, i.e., we change an implementation dependent piece of logic to contract dependent logic that always works, thereby removing the need for implementing this again and again for new bootstrap algorithms.

[fkiraly]

I think this PR flags up an open design discussion that we need to close out.

What are our convention on the n_bootstraps parameter? I get that every bootstrap algorithm needs to have that. (is that true?)

There are potential conditions we can impose: A. all parameters must have defaults (sklearn requires this, sktime does not) B. n_bootstraps must be the first parameter C. n_bootstraps always must have the same default, 10

You cannot have not-A, but B and C.

I thought the convention was A and B and C, but there is one bootstrap that does not satisfy it, the BlockStatisticPreservingBootstrap.

So either we need to adopt a convention which is not (A and B and C), or change the signature of BlockStatisticPreservingBootstrap.

[astrogilda]

I think this PR flags up an open design discussion that we need to close out.

What are our convention on the n_bootstraps parameter? I get that every bootstrap algorithm needs to have that. (is that true?)

There are potential conditions we can impose: A. all parameters must have defaults (sklearn requires this, sktime does not) B. n_bootstraps must be the first parameter C. n_bootstraps always must have the same default, 10

You cannot have not-A, but B and C.

I thought the convention was A and B and C, but there is one bootstrap that does not satisfy it, the BlockStatisticPreservingBootstrap.

So either we need to adopt a convention which is not (A and B and C), or change the signature of BlockStatisticPreservingBootstrap.

Good observation. I would prefer we go with the latter -- change the signature of the one errant class.

[fkiraly]

to what? I.e., what is a sensible default in your opinion?

[astrogilda]

I was thinking 100?

On Wed, Apr 17, 2024, 3:56 AM Franz Király @.***> wrote:

to what? I.e., what is a sensible default in your opinion?

— Reply to this email directly, view it on GitHub https://github.com/astrogilda/tsbootstrap/pull/136#issuecomment-2060635213, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFTOOHWB3YRZ7LVGN2CHSNLY5YTKJAVCNFSM6AAAAABGJXMA4GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRQGYZTKMRRGM . You are receiving this because you commented.Message ID: @.***>

[fkiraly]

I meant, for block_bootstrap, not for n_bootstraps which already has a default everywhere?

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.25%. Comparing base (5d53806) to head (5c4fd9b). Report is 73 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #136 +/- ## ========================================== + Coverage 84.77% 88.25% +3.48% ========================================== Files 17 31 +14 Lines 2607 2903 +296 ========================================== + Hits 2210 2562 +352 + Misses 397 341 -56 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.