[ENH] tsbootstrap adapter
#5887
Conversation
benHeid
added
module:transformations
|
|
||
| Parameters | ||
| ---------- | ||
| tsbootstrapper : Bootstrapper from tsbootstrap |
There was a problem hiding this comment.
too long variable name imo
There was a problem hiding this comment.
Is bootstrapper short enough?
There was a problem hiding this comment.
maybe bootstrap?
(that's the shorthand and scitype name I've used in the refactor,, and less strange in English than "bootstrapper")
| "enforce_index_type": None, # index type that needs to be enforced in X/y | ||
| "fit_is_empty": True, # is fit empty and can be skipped? Yes = True | ||
| "transform-returns-same-time-index": False, | ||
| "python_dependencies": "tsbootstrap", |
There was a problem hiding this comment.
Do you mean something like: "tsbootstrap>=0.1.0"? I tested it with "tsbootstrap>=0.0.32 and this is failing...
Not sure, but I think the dependency check is not correct here: https://github.com/sktime/sktime/blob/63a95f2c29251575b7963ad269044654cbe9d96c/sktime/utils/validation/_dependencies.py#L177C1-L178C1
| bootstrapped_samples, index=multi_index, columns=X.columns | ||
| ) | ||
|
|
||
| X.index = pd.MultiIndex.from_product([["original"], X.index]) |
There was a problem hiding this comment.
do we always want to include the original series?
I think it should be at most optional, parameter controlled, default=no
There was a problem hiding this comment.
Mhm.. I used the behavior of the STLBootstrap as template. This bootstrapper is including the original values (called actual, I would rename to actual in this PR)
from sktime.transformations.bootstrap import STLBootstrapTransformer
from sktime.datasets import load_airline
from sktime.utils.plotting import plot_series # doctest: +SKIP
y = load_airline() # doctest: +SKIP
transformer = STLBootstrapTransformer(10) # doctest: +SKI
y_hat = transformer.fit_transform(y) Number of airline passengers
series_id time_index
actual 1949-01 112.000000
1949-02 118.000000
1949-03 132.000000
1949-04 129.000000
1949-05 121.000000
... ...
synthetic_9 1960-08 630.055938
1960-09 505.544319
1960-10 459.539478
1960-11 407.284782
1960-12 469.909948
There was a problem hiding this comment.
Mhm.. I used the behavior of the
STLBootstrapas template. This bootstrapper is including the original values (calledactual, I would rename toactualin this PR)
Yes, but I think that is a special choice rather than user expected, and it complicates the interface. It is also not sth that tsbootstrap does (and imo it's the better design choice) - FYI @astrogilda
There was a problem hiding this comment.
I will take a look on the other bootstrapped in sktime and check how they are behaving. But in that case, I think it would be good to streamline the behavior using deprecation cycles.
There was a problem hiding this comment.
you are probably right. Safest perhaps is add a parameter include_actual or similar and switch its default.
There was a problem hiding this comment.
should we open an issue? I am guessing that all bootstrapping transformers are in the bootstrap folder.
| f"synthetic_{i}" | ||
| for i in range(self.tsbootstrapper.config.n_bootstraps) | ||
| ], | ||
| X.index, |
There was a problem hiding this comment.
this will break, since in genera the bootstrap samples are shorter.
I would leave range index if we cannot come up with sth better.
|
How should we deal with this failure?
|
benHeid
force-pushed
the
tsbootstrap_adapter
branch
from
c3dcede
to
7cdcbf5
Compare
|
updated source after |
| """ | ||
| from sktime.utils.validation._dependencies import _check_soft_dependencies | ||
|
|
||
| deps = cls.get_class_tag("python_dependencies") |
There was a problem hiding this comment.
Just observing it's probably being done directly against the package name in other adapters. But this may be better, as future changes in dependencies won't affect.
There was a problem hiding this comment.
yes, we should do _check_estimator_deps(self)
There was a problem hiding this comment.
self is not available since get_test_params is a classmethod. However, we can instanitate a tsbootstrap for testing.. But I do not really like to do that.
There was a problem hiding this comment.
oh, then cls works too, as arg. No need to instantiate.
|
Yes, that's weird. It displayed as still running, but GHA behaved as if it were failed. I restarted, and now it's gone. |
Reference Issues/PRs
Do not merge before release related issues are fixed in astrogilda/tsbootstrap
closes #5880
closes astrogilda/tsbootstrap#45
What does this implement/fix? Explain your changes.
Add an adapter for tsbootstrap
Does your contribution introduce a new dependency? If yes, which one?
tsbootstrap
What should a reviewer concentrate their feedback on?
Did you add any tests for the change?
Any other comments?
PR checklist
For all contributions
How to: add yourself to the all-contributors file in the
sktimeroot directory (not theCONTRIBUTORS.md). Common badges:code- fixing a bug, or adding code logic.doc- writing or improving documentation or docstrings.bug- reporting or diagnosing a bug (get this pluscodeif you also fixed the bug in the PR).maintenance- CI, test framework, release.See here for full badge reference
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.python_dependenciestag and ensureddependency isolation, see the estimator dependencies guide.