Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: scikit-base registry and testing framework #26

Merged
merged 79 commits into from
Feb 24, 2024

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented Jan 6, 2024

Depends on #10 which should be merged first. Implements #9

Adds scikit-base machinery, for sktime compatibility.
Contents:

  • registry with lookup utility all_objects, including tests
  • BaseObject interface contract test class, TestAllObjects
  • test scenarios for bootstrap algorithms
  • TestAllBootstraps a test class specific to bootstrap objects, currently testing input/output contract of the bootstrap method
  • new tag capability:multivariate, which distinguishes boostrap algorithms that can vs cannot handle multivariate data. Used in an informative user facing error message, and matching algorithms with test scenarios.

One open question is, where should tests live?
I added tests currently in a sub-folder of tsbootstrap rather than the current tests folder, but that's not necessary.

@fkiraly fkiraly added the enhancement New feature or request label Jan 6, 2024
@fkiraly fkiraly linked an issue Jan 6, 2024 that may be closed by this pull request
@astrogilda
Copy link
Owner

Depends on #10 which should be merged first. Implements #9

Adds scikit-base machinery, for sktime compatibility. Contents:

  • registry with lookup utility all_objects, including tests
  • BaseObject interface contract test class, TestAllObjects
  • test scenarios for bootstrap algorithms
  • TestAllBootstraps a test class specific to bootstrap objects, currently testing input/output contract of the bootstrap method

One open question is, where should tests live? I added tests currently in a sub-folder of tsbootstrap rather than the current tests folder, but that's not necessary.

What's the rationale behind changing the location for the tests folder?

@fkiraly
Copy link
Collaborator Author

fkiraly commented Jan 20, 2024

technically, I've added it, not changed it. I just did not know for certain where to put it. No strong opinions where it should go.

@fkiraly
Copy link
Collaborator Author

fkiraly commented Feb 19, 2024

2nd one is resolved, the issue was:

  • not all bootstrap algorithms accept multivariate input, i.e., shape (n_inst, n_vars) with n_vars>1.
  • the exception that was supposed to be raised was internally silenced (try/except/print, not try/except/raise)

This is solved by:

  • adding a tag, and testing dependent on the tag
  • moving the multivariate check to _check_input

@fkiraly
Copy link
Collaborator Author

fkiraly commented Feb 20, 2024

I've investigated the first issue, and the failure is not, as I thought, about generators or not. The problem is that if return_index=True, then the return generator produces (non-generator) returns that are non-compliant with specification, namely tuples of shape

(list-of[list-of[np.ndarray]], list-of[np.ndarray]), and it looks like the second tuple element is the bootstrapped series, not the first.

Reproducing code:

from tsbootstrap.block_bootstrap import BlackmanBootstrap
from tsbootstrap.tests.scenarios.scenarios_bootstrap import BootstrapUnivarRetIx

boot = BlackmanBootstrap()
sc = BootstrapUnivarRetIx()

# this is the same as running the blackman bootstrap
# with some X, y, and return_indices=True
result_gen = sc.run(boot, method_sequence=["bootstrap"])

result_list = list(result_gen)  # then investigate this list

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 77.06186% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 88.59%. Comparing base (5d53806) to head (83d5b65).
Report is 32 commits behind head on main.

Files Patch % Lines
src/tsbootstrap/tests/scenarios/scenarios.py 68.11% 22 Missing ⚠️
src/tsbootstrap/utils/estimator_checks.py 19.04% 17 Missing ⚠️
...rc/tsbootstrap/tests/scenarios/scenarios_getter.py 63.15% 14 Missing ⚠️
src/tsbootstrap/tests/test_class_register.py 26.31% 14 Missing ⚠️
src/tsbootstrap/registry/_lookup.py 71.42% 8 Missing ⚠️
src/tsbootstrap/utils/dependencies.py 77.77% 4 Missing ⚠️
src/tsbootstrap/tests/test_all_estimators.py 92.85% 3 Missing ⚠️
src/tsbootstrap/registry/tests/test_tags.py 88.88% 2 Missing ⚠️
...tsbootstrap/tests/scenarios/scenarios_bootstrap.py 96.55% 2 Missing ⚠️
src/tsbootstrap/tests/test_switch.py 84.61% 2 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
+ Coverage   84.77%   88.59%   +3.82%     
==========================================
  Files          17       31      +14     
  Lines        2607     2763     +156     
==========================================
+ Hits         2210     2448     +238     
+ Misses        397      315      -82     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fkiraly fkiraly merged commit 6c2993b into astrogilda:main Feb 24, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add skbase suite tests and ensure current bootstrap classes pass
3 participants