feat/3037 phase 1 refactor return types#3131
Open
ozink-u8 wants to merge 32 commits into
Open
Conversation
6fbdb01 to
141684c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3131 +/- ##
==========================================
- Coverage 96.55% 96.49% -0.07%
==========================================
Files 160 160
Lines 17285 17294 +9
==========================================
- Hits 16689 16687 -2
- Misses 596 607 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist before merging this PR:
Acts as a preparation for #3037.
Summary
Problem:
Many functions accepting
TimeSeriesLikemanually handle the single-vs-sequence type handling. The canonical example (a few sites vary slightly) is:Change:
Replace that scattered logic with a single centralized exit through
series2seq(...):Funnelling all single-vs-sequence handling through series2seq not only creates a single source of truth, which also eases the planned TSS introducing changes (Phase 2).
Other Information
ts_utils.pyfortytypechecking.Out of Scope
anomaly_model.py::score, returns a list[tuple[TS, ...]] for each scorer.invertible_data_transformer.py::inverse_transformdeals with historical forecasts, therefore of type SEQ_SEQ and flattens and reconstructs them.forecasting_model.py::historical_forecasts: SEQ_SEQ touchpoint with lpo=Falseforecasting_model.py::residuals: Uses HFC therefore SEQ_SEQ touchpoint with lpo=Falseconformal_models.py::residuals: Uses HFCconformal_models.py::_calibrate_forecastsUses residuals -> HFChistorical_forecasts/utils.py::_apply_inverse_data_transformers: Touches SEQ_SEQ with lpo=False. Right now in this caseseries2seqjust leaves SEQ_SEQ unchanged.historical_forecasts/utils.py:: _process_historical_forecast_for_backtestUses HFC -> SEQ_SEQdatasets.pyMany different datasets have hardcoded list[TS] as return type -> Would change to TSS in Phase 2 (Electricity, UberTLC, ILINet, ExchangeRate, Traffic, Weather)model_selection.py::make_splitter: Returns tuple for train, test of list/SplitTimeSeriesSequence objects. Which violates our rule of outer list being different TSs -> Intentionally left outbase_data_transformer.py::{apply_component_mask, unapply_component_mask}: Mixed return types including np.ndarray -> Unclear if should be refactored in Phase 2metrics/utils.pyUses Literals as return not TS -> not relevant for TSS changes