-
Notifications
You must be signed in to change notification settings - Fork 78
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.
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
[FEAT,BREAKING CHANGE] Add PERMBU integration to HierarchicalReconciliation class #83
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -10,7 +10,7 @@ | |||
[![PyPi](https://img.shields.io/pypi/v/hierarchicalforecast?color=blue)](https://pypi.org/project/hierarchicalforecast/) | |||
[![License](https://img.shields.io/badge/License-Apache_2.0-blue.svg)](https://github.com/Nixtla/hierarchicalforecast/blob/main/LICENSE) | |||
|
|||
**HierarchicalForecast** offers a collection of reconciliation methods, including `BottomUp`, `TopDown`, `MiddleOut`, `MinTrace` and `ERM`. And Probabilistic coherent predictions including `MinTrace-normality`, `Bootstrap`, and `PERM-BU`. | |||
**HierarchicalForecast** offers a collection of reconciliation methods, including `BottomUp`, `TopDown`, `MiddleOut`, `MinTrace` and `ERM`. And Probabilistic coherent predictions including `Normality`, `Bootstrap`, and `PERMBU`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is cool, nbdev has an automated link generation if the module in quotations '``' matches something in the library.
**Returns:**<br> | ||
`y_tilde`: pd.DataFrame, with reconciled predictions. | ||
""" | ||
if intervals_method not in ['normality', 'bootstrap', 'permbu']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 'normality' method refering to 'mint_normality'?
I think the name could be a source of confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @kdgutier! I've changed the name to normality
since it is a general method that works independently of the reconciliation method: https://otexts.com/fpp3/rec-prob.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, would it be possible to explain it in the docstring with the pointer to the link?
hierarchicalforecast/methods.py
Outdated
return _reconcile(S, P, W, y_hat, sigmah=sigmah, level=level, | ||
bootstrap=bootstrap, bootstrap_samples=bootstrap_samples) | ||
class Bootstrap: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add references to Gamakumara's work for this class, in its docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the description of the algorithm in similar fashion to that of PERMBU would be of interest to our power users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also add some warning to PERMBU and Bootstrap on the importance of not overfitted residuals.
hierarchicalforecast/methods.py
Outdated
temp = array.argsort(axis=1) | ||
ranks = np.empty_like(temp) | ||
a_range = np.arange(temp.shape[1]) | ||
for iRow in range(temp.shape[0]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep caps for classes and underscores for other variables i_row?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I can change it. To be completely transparent, I didn't change anything in the PERMBU class. I just made it compatible. The iRow
variable was merged into the main branch without review in a previous PR.
hierarchicalforecast/methods.py
Outdated
hier_levels = hier_links.shape[1]-1 | ||
for level_idx in reversed(range(hier_levels)): | ||
# Obtain aggregation matrix from parent/children links | ||
children_links = np.unique(hier_links[:,level_idx:level_idx+2], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is an overlap between these children_link lines and the _get_child_nodes
function.
warnings.warn('Replacing negative forecasts with zero.') | ||
y_hat = np.copy(y_hat) | ||
y_hat[y_hat < 0] = 0. | ||
# Quadratic progamming formulation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
progamming and origial
@@ -629,39 +753,7 @@ def __init__(self, | |||
self.nonnegative = nonnegative | |||
self.insample = False | |||
|
|||
def reconcile(self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not seen the documentation, but will showdoc(OptimalCombination.fit)
work as previous?
Or will it have MinTrace
hierarchicalforecast/methods.py
Outdated
@@ -59,23 +64,243 @@ def _reconcile(S: np.ndarray, P: np.ndarray, W: np.ndarray, | |||
res[f'hi-{lv}'] = res['mean'] + zs * sigmah | |||
return res | |||
|
|||
# %% ../nbs/methods.ipynb 5 | |||
class Normality: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this MinTraceNormality?
It would be good to add docstrings, with the shapes of sigmah is it a matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name to normality since it is a general method that works independently of the reconciliation method: https://otexts.com/fpp3/rec-prob.html
@@ -1,530 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we add the following reference:
And the following paragraph at the beginning of the notebook:
In this notebook we will explore probabilistic hierarchical sample reconciliation using the PERM-BU method. The PERM-BU (Taieb et. al, 2017) method leverages empirical bottom-level marginal distributions with empirical copula functions (describing bottom-level dependencies) to generate the distribution of the aggregate-level distributions using the bottom-up reconciliation. The sample reordering technique from the PERM-BU method reinjects multivariate dependencies into independent bottom-level samples.
sktime-disclaimer
…rarchicalforecast into feat/permbu-integration
This PR integrates the PERMBU method to the general class HierarchicalReconciliation.
The integration works for all compatible reconciliation methods (not just BottomUp).
A usage example was also included.