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.

Sign up for GitHub

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

Jump to bottom

fix/historical_fore_fut_cov_notarget_lags #1685

Merged
merged 11 commits into from
Apr 2, 2023
Merged

fix/historical_fore_fut_cov_notarget_lags #1685

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
83ee28b
correction
dumjax Apr 1, 2023
501081c
Delete test_.py
dumjax Apr 1, 2023
8caf166
corrections
dumjax Apr 2, 2023
d4d7efb
corrections
dumjax Apr 2, 2023
4a7f648
corrections
dumjax Apr 2, 2023
421f7b0
corrections
dumjax Apr 2, 2023
8e0c748
corrections docstring of
dumjax Apr 2, 2023
04c9833
Update darts/models/forecasting/forecasting_model.py
dumjax Apr 2, 2023
b332d04
Update darts/models/forecasting/forecasting_model.py
dumjax Apr 2, 2023
6213d47
corrections docstring of extreme_lags
dumjax Apr 2, 2023
2599299
corrections docstring of extreme_lags
dumjax Apr 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 14 additions & 15 deletions darts/models/forecasting/forecasting_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ def _predict_sample_time_index_length(self) -> int:

def _get_historical_forecastable_time_index(
self,
series: Optional[TimeSeries] = None,
series: TimeSeries,
past_covariates: Optional[TimeSeries] = None,
future_covariates: Optional[TimeSeries] = None,
is_training: Optional[bool] = False,
Expand All @@ -392,7 +392,7 @@ def _get_historical_forecastable_time_index(
Parameters
----------
series
Optionally, a target series.
A target series.
past_covariates
Optionally, a past covariates.
future_covariates
Expand Down Expand Up @@ -440,20 +440,19 @@ def _get_historical_forecastable_time_index(
max_future_cov_lag,
) = self.extreme_lags

intersect_ = None
if min_target_lag is None:
min_target_lag = 0

# target longest possible time index
if (min_target_lag is not None) and (series is not None):
intersect_ = generate_index(
start=series.start_time()
- (min_target_lag - max_target_lag) * series.freq
if is_training
else series.start_time() - min_target_lag * series.freq,
end=series.end_time(),
freq=series.freq,
)
# longest possible time index for target
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :) Could you add some unit tests which check the issue?
It's always good when fixing a bug to add a unit test, because we apparently haven't checked it before

intersect_ = generate_index(
start=series.start_time() - (min_target_lag - max_target_lag) * series.freq
if is_training
else series.start_time() - min_target_lag * series.freq,
end=series.end_time(),
freq=series.freq,
)

# past covariates longest possible time index
# longest possible time index for past covariates
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the past covariates generated indexes cover all use cases:

  • What happens with RegressionModel(lags_past_covariates=[-100], output_chunk_length=1)?
    The user supplied past_covariates for fit()can actually be much shorter than the target series (101 steps shorter, I believe).
    With the current logic and the shorter past_covariates, the intersection would result in a much shorter historic_forecastable_time_index (because of the end=past_covariates.end_time()). We probably need the max_past_cov_lag as well.
    I think for future covariates it is correct, so similarly including the max_past_cov lag should probably work
  • also is the method supposed to also give the predictable indices after the end (when is_training=False) taking into account the output_chunk_length/max_target_lag? Then we would have to add the max_target_lag to all end parameters

if (min_past_cov_lag is not None) and (past_covariates is not None):
tmp_ = generate_index(
start=past_covariates.start_time()
Expand All @@ -469,7 +468,7 @@ def _get_historical_forecastable_time_index(
else:
intersect_ = tmp_

# future covariates longest possible time index
# longest possible time index for future covariates
if (min_future_cov_lag is not None) and (future_covariates is not None):
tmp_ = generate_index(
start=future_covariates.start_time()
Expand Down