Fix/Make reconcilliation order independent #1592
Conversation
There was a problem hiding this comment.
LGTM to me... can't believe I missed this in the initial implementation 🤦
Many thanks @DavidKleindienst !
@dennisbader we will want this in the next release.
| @@ -179,3 +180,43 @@ def test_more_intricate_hierarchy(self): | |||
| recon = MinTReconciliator("wls_val") | |||
| recon.fit(self.series_complex) | |||
| self._assert_reconciliation_complex(recon) | |||
|
|
|||
| def test_reconcilliation_is_order_independent(self): | |||
There was a problem hiding this comment.
Many thanks for adding the tests too 👍
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1592 +/- ##
==========================================
+ Coverage 94.02% 94.04% +0.01%
==========================================
Files 125 125
Lines 11088 11171 +83
==========================================
+ Hits 10426 10506 +80
- Misses 662 665 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
* unit test for component order independence * remove incorrect assumption from test * Fix get_projection_matrix * minor improvement --------- Co-authored-by: David Kleindienst <kleindienst@ximes.com> Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
Fixes #1582.
Summary
Fixes
get_projection_matrixinBottomUpReconciliatorandTopDownReconcilliatorsuch that the result takes the order of components of theTimeSeriesinto account.Adds a test to assert the results of
BottomUpReconciliator,TopDownReconciliatorandMinTReconciliatordo not depend on component orderOther Information