Create BaseReconciler
#1054
Create BaseReconciler
#1054
Conversation
Codecov Report
@@ Coverage Diff @@
## hierarchical_pipeline #1054 +/- ##
========================================================
Coverage ? 86.37%
========================================================
Files ? 166
Lines ? 8839
Branches ? 0
========================================================
Hits ? 7635
Misses ? 1204
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
🚀 Deployed on https://deploy-preview-1054--etna-docs.netlify.app |
| if ts.current_df_level != self.source_level: | ||
| raise ValueError(f"Dataset should be on the {self.source_level} level!") | ||
|
|
||
| current_level_segments = ts.hierarchical_structure.get_level_segments(level_name=self.source_level) |
There was a problem hiding this comment.
Maybe move this lines before check? In such case, we will check that target and source levels are parts of the hierarchy and, after that, we will check that dataset at the valid level.
There was a problem hiding this comment.
Call of this methods requires some movements inside)
I think it is better to check the more explicit requirements to the input data(has hierarchy/current level = source level...)
Also the code structure is better now as all the checks are at the beginning and are separate from the other logic
| ts_aggregated = ts.get_level_dataset(target_level=self.source_level) | ||
| return ts_aggregated | ||
|
|
||
| def reconcile(self, ts: TSDataset) -> TSDataset: |
There was a problem hiding this comment.
I think here we also need to check if the target level is higher than the source.
There was a problem hiding this comment.
It is not required here as this method is common for both top-down and bottom-up
| target_level_segments = ts.hierarchical_structure.get_level_segments(level_name=self.target_level) | ||
|
|
||
| current_level_values = ts[:, current_level_segments, "target"].values | ||
| target_level_values = current_level_values @ self.mapping_matrix.T |
There was a problem hiding this comment.
Will it work properly with series that starts at different timestamps?
There was a problem hiding this comment.
It should, we do the same thing here and have tests with nans
There was a problem hiding this comment.
That is why I thought of moving this logic to get_level_dataset as it is almost the same
| def test_aggregate_fails_low_source_level(hierarchical_ts): | ||
| ts_market_level = hierarchical_ts.get_level_dataset(target_level="market") | ||
| reconciliator = DummyReconciliator(target_level="level", source_level="product") | ||
| with pytest.raises(ValueError, match=""): |
There was a problem hiding this comment.
Can't we check error message more specifically?
There was a problem hiding this comment.
My fail)
|
|
||
| def test_reconcile_not_fitted_fails(hierarchical_ts): | ||
| reconciliator = DummyReconciliator(target_level="level", source_level="product") | ||
| with pytest.raises(ValueError, match=f"Reconciliator is not fitted!"): |
There was a problem hiding this comment.
Do we need f-string here?
There was a problem hiding this comment.
We don't)
Before submitting (must do checklist)
Proposed Changes
Closing issues
closes #1032