[BUG] Fix test errors for pytorch-forecasting estimators#10415
Conversation
CloseChoice
left a comment
CloseChoice
left a comment
There was a problem hiding this comment.
just some notes for me. Want to see the CI output before doing more changes though.
| test_fun(**deepcopy(args)) | ||
| results[key] = "PASSED" | ||
| print_if_verbose("PASSED") | ||
| results[key] = "PASSED" |
There was a problem hiding this comment.
probably not wanted!
| random_log_path: bool (default=False) | ||
| use random root directory for logging. This parameter is for CI test in | ||
| Github action, not designed for end users. | ||
| model_params : dict[str, Any] (default=None) |
There was a problem hiding this comment.
we should reset this!
| # ----------------- | ||
| "tests:core": True, # should tests be triggered by framework changes? | ||
| "tests:skip_all": True, | ||
| # the fitted pytorch-forecasting model caches a function-local class |
There was a problem hiding this comment.
If we are going to skip these tests, why not move them to the _PytorchForecastingAdapter? That would save us from copying it to each and every class, and the test-suite would always skip these tests for all pytorch-forecasting models?
Although not sure if this is a "safe" way to add skips as it would lie in one layer above the end point
|
I think we should have some substitute for these saving tests where we check for |
|
Also, I am surprised why |
I agree, though would postpone this to later. |
phoeenniixx
added
bug
…pter _origin_time_idx is a transient lookup (internal int time_idx -> original index, filtered to the prediction window) built in _Xy_to_dataset. It was stored on self and re-assigned on every fit/predict call, so predict changed the estimator __dict__ (training window vs prediction window), tripping test_non_state_changing_method_contract. Thread it through as a return value of _Xy_to_dataset and an argument to _predictions_to_dataframe instead of storing it on self. _fit discards it. This removes the instance mutation entirely rather than backing up/restoring a value that is never read across calls. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fkiraly
changed the title
pytorch-forecasting estimators
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve failing tests for the pytorch-forecasting-based forecasters in sktime, primarily by improving test determinism/reproducibility and by skipping persistence-related estimator tests that are incompatible with the underlying model objects.
Changes:
- Add a
deterministicoption to pytorch-forecasting forecasters and set it in test params to stabilize test outcomes. - Adjust the pytorch-forecasting adapter to handle loss/callbacks separately and to avoid estimator state mutation during prediction by returning transient index lookup data.
- Skip pickling/persistence tests for these estimators and ignore model checkpoint directories in git.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
sktime/forecasting/pytorchforecasting.py |
Adds deterministic parameter, updates doctest directives, and tags persistence-related tests to be skipped for pytorch-forecasting estimators. |
sktime/forecasting/base/adapters/_pytorchforecasting.py |
Refactors adapter internals for loss/callback handling, deterministic seeding, and non-state-mutating prediction indexing. |
.gitignore |
Ignores checkpoints/ directory to prevent accidental commits of model artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._model_loss = model_params.pop("loss", None) if model_params else None | ||
| self._model_params = deepcopy(model_params) if model_params else {} | ||
| self._callbacks = trainer_params.pop("callbacks", None) | ||
| self._trainer_params = ( | ||
| deepcopy(trainer_params) if trainer_params is not None else {} | ||
| ) |
| # if fh is not continuous, there will be NaN after extend_y in prediect | ||
| data = data.copy() | ||
| data["_target_column"] = data["_target_column"].fillna(0) | ||
| data["_target_column"] = data["_target_column"].fillna(0) # .astype(float) |
| deterministic: bool (default=False) | ||
| set seed before predict, so that it will give the same output for the same input |
| deterministic: bool (default=False) | ||
| set seed before predict, so that it will give the same output for the same input |
| deterministic: bool (default=False) | ||
| set seed before predict, so that it will give the same output for the same input | ||
| random_log_path: bool (default=False) | ||
| use random root directory for logging. This parameter is for CI test in | ||
| Github action, not designed for end users. | ||
| deterministic: bool (default=False) | ||
| set seed before predict, so that it will give the same output for the same input |
| @@ -1008,6 +1051,7 @@ def __init__( | |||
| random_log_path, | |||
| broadcasting, | |||
| ) | |||
| self.deterministic = deterministic | |||
|
|
|||
Reference Issues/PRs
reconciles the prs ##10412 and #10383
What does this implement/fix? Explain your changes.
Does your contribution introduce a new dependency? If yes, which one?
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
maintainerstag - do this if you want to become the owner or maintainer of an estimator you added.See here for further details on the algorithm maintainer role.
For new estimators
docs/source/api_reference/taskname.rst, follow the pattern.Examplessection.python_dependenciestag and ensureddependency isolation, see the estimator dependencies guide.