A few small follow-ups from recent PRs that mostly fall in the "good first issue" bucket. I'm opening them here as a single umbrella so they can be claimed and worked in parallel instead of buried in the individual PR threads.
1. Stale docstring in TreePredictor.deserialize
src/gluonts/ext/rotbaum/_predictor.py around line 367:
def deserialize(cls, path: Path, **kwargs: Any) -> \"TreePredictor\":
\"\"\"
...
It loads the predictor class with the serialized arguments. It then
loads the trained model list by reading the pickle file.
\"\"\"
After #3176, the method reads model_list.json, not predictor.pkl. The docstring should match. (#3294 touches this line as part of its fallback change; if that PR lands first this item is resolved.)
2. Regression coverage the recent PRs didn't add
Each of these PRs changed an observable behaviour but didn't pin it in a test; the list is short enough that each one is a small, self-contained test file:
Not asking for anything cosmetic
Each bullet above was surfaced while validating a substantive behaviour change; none are style nits. Happy to send the docstring fix now and the test additions as separate small PRs if maintainers would like them split up by topic. Flagging good first issue since the scope per bullet is small and the relevant PR already explains the intended behaviour.
A few small follow-ups from recent PRs that mostly fall in the "good first issue" bucket. I'm opening them here as a single umbrella so they can be claimed and worked in parallel instead of buried in the individual PR threads.
1. Stale docstring in
TreePredictor.deserializesrc/gluonts/ext/rotbaum/_predictor.pyaround line 367:After #3176, the method reads
model_list.json, notpredictor.pkl. The docstring should match. (#3294 touches this line as part of its fallback change; if that PR lands first this item is resolved.)2. Regression coverage the recent PRs didn't add
Each of these PRs changed an observable behaviour but didn't pin it in a test; the list is short enough that each one is a small, self-contained test file:
trainer.fitraising aValueErroron a tiny model leavesbest_model_pathpopulated and the returned predictor usable. Without this, the broadexcept Exceptiondiscussed in Torch estimator: broad except Exception in checkpoint recovery masks unrelated failures and loses optimizer/RNG state #3295 cannot regress visibly.observed_value_field: Optional[str]: no test passingNonethroughTFTInstanceSplitteron a toy dataset. The happy path with the default field name is covered; the newNonebranch isn't.scaling: Optional[str]: no test exercisingscaling=None(and, after Torch: reject unknown scaling values instead of silent NOPScaler #3293, no test asserting that unknown scaling strings raiseValueError). A single parametrize acrosspatch_tst,lag_tst,d_linear,tidewould cover all four estimators.test/dataset/test_tsf_reader.pypins the mapping, but there is no test asserting that the emitted strings don't triggerFutureWarningon a supported pandas (i.e. no contract test against pandas 2.x). Datasets: finish pandas 2 freq alias migration in _tsf_reader #3291 fixes the values; a follow-up test that fails loudly if anyone re-adds\"M\"/\"Q\"/\"Y\"/\"S\"would be worth adding alongside.QuantileOutput: no test instantiatingTiDEEstimator(distr_output=QuantileOutput(...))throughcreate_lightning_module(). The existing tests exerciseStudentTOutput()only.past_time_feat/future_time_featwhennum_feat_dynamic_real > 0. Current tests keep it at the default0.Not asking for anything cosmetic
Each bullet above was surfaced while validating a substantive behaviour change; none are style nits. Happy to send the docstring fix now and the test additions as separate small PRs if maintainers would like them split up by topic. Flagging
good first issuesince the scope per bullet is small and the relevant PR already explains the intended behaviour.