Implement-median-model-v4 #793
Open
+859
−1
Conversation
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
egordm
approved these changes
Jan 8, 2026
Collaborator
egordm
left a comment
egordm
left a comment
There was a problem hiding this comment.
Great changes. I have only a few nipicks about test and possibly moving some logic to timeseries dataset.
| load_lag_PT1H=[1.0, np.nan, np.nan], | ||
| load_lag_PT2H=[4.0, 1.0, np.nan], | ||
| load_lag_PT3H=[7.0, 4.0, 1.0], | ||
| available_at=index, |
Collaborator
There was a problem hiding this comment.
Usually forecast input dataset no longer has an available at. So this could be removed for simplicity. If input dataset as an extra timestamp then it's horizon but in this case only one is supported.
Comment on lines
+187
to
+222
| def _infer_frequency(index: pd.DatetimeIndex) -> pd.Timedelta: | ||
| """Infer the frequency of a pandas DatetimeIndex if the freq attribute is not set. | ||
|
|
||
| This method calculates the most common time difference between consecutive timestamps, | ||
| which is more permissive of missing chunks of data than the pandas infer_freq method. | ||
|
|
||
| Args: | ||
| index (pd.DatetimeIndex): The datetime index to infer the frequency from. | ||
|
|
||
| Returns: | ||
| pd.Timedelta: The inferred frequency as a pandas Timedelta. | ||
|
|
||
| Raises: | ||
| ValueError: If the index has fewer than 2 timestamps. | ||
| """ | ||
| minimum_required_length = 2 | ||
| if len(index) < minimum_required_length: | ||
| raise ValueError("Cannot infer frequency from an index with fewer than 2 timestamps.") | ||
|
|
||
| # Calculate the differences between consecutive timestamps | ||
| deltas = index.to_series().diff().dropna() | ||
|
|
||
| # Find the most common difference | ||
| return deltas.mode().iloc[0] | ||
|
|
||
| def _frequency_matches(self, index: pd.DatetimeIndex) -> bool: | ||
| """Check if the frequency of the input data matches the model frequency. | ||
|
|
||
| Args: | ||
| index (pd.DatetimeIndex): The input data to check. | ||
|
|
||
| Returns: | ||
| bool: True if the frequencies match, False otherwise. | ||
| """ | ||
| input_frequency = self._infer_frequency(index) if index.freq is None else index.freq | ||
| return input_frequency == self.frequency |
Collaborator
There was a problem hiding this comment.
Maybe it would be nice to move this to TimeSeriesDataset. To have something like one function called validate_sample_interval that checks the data against the set sample interval. If user wants to be sure they can call it.
It would make it easier to test and median model code would be a lot simpler.
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.
Changes proposed in this PR include: