-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH] HierarchyEnsembleForecaster
for level- or node-wise application of forecasters on panel/hierarchical data
#3905
Conversation
neat! let us know when you would like a review |
Hi @fkiraly, this PR is ready for review now. Thanks. |
excellent! Will start the CI and we'll see if anything fails. |
the doctest is failing, since it checks actual printout against expected. When you run the line the printout is You can catch that by adding the line
|
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.
Great, looks good!
Some small things:
- can you kindly fix the doctest? Explanation is above.
- there is a merge conflict with the contributors file, kindly fix
hm, these look like genuine failures when you are trying to hash your node dict |
recommendation: run your tests locally! |
HierarchyEnsembleForecaster
for panel/hierarchical data
Its strange but the tests didn't fail locally. Fixed the doctsring error, but still not able to recreate the unhashable type failure. |
how odd. Should we try to run them again? I'll restart. There is also a merge conflict in the contributors file, kindly update from main. |
8f2d349
to
c2eb974
Compare
added hier-ensm and init files
added test file
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.
These are genuine failures now.
Have you run check_estimator
locally and tried to debug?
I would recommend that, see here:
https://www.sktime.org/en/stable/developer_guide/add_estimators.html
Hi @fkiraly, I have the updated the code and majority of the issues are fixed. But still, some tests (around 12) are failing due to a common error :- ValueError('Length of names must match number of levels in MultiIndex.'). This error is inevitable for a particular test instance, the way the check_estimator tests are designed currently. Why I think so? The above error is linked with all the test instances with by = 'node'. Ideally, the length of a particular node being passed in 'forecasters' attributes should be N-1, where N is the levels of multiindex data. (The last level of the data is assumed to be a timepoint index and hence N-1). The way the tests are designed, the hier-ensm forecaster will require different length of nodes being passed in 'forecasters' argument for different category of data (for eg univariate and panel data). Since, nodes can only be specified once for a particular category of data before running the tests, it fails for the other categories. Could you please give some suggestions on how to handle this issue? |
@VyomkeshVyas, sorry for the delay in the reply. I was fixing some merge conflicts and doctest errors in this PR so the tests would run through to the failures that you are referring to (hope that's ok). Once I can see the failures, I'll have a look. |
Thanks for the explanation! Indeed, as the test framework is designed, all inputs are passed to all forecasters, which in this case upsets the new forecaster, as its parameters need to match the levels. I see multiple solutions:
|
@fkiraly Thanks for the suggestions! That's very helpful. |
I forgot to add aggregation levels in X, y in update and predict functions. I am working on it. |
i.e., you went with option 2, right?
Let us know when you think this is ready. |
yea, with option 2. But, instead I am not ignoring the mismatched node, rather grouping all the nodes which are super set of mismatch node.
Its ready now. Thanks again. |
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.
Excellent! Impressive for a first contribution!
To be frank, when I saw the first version and the specs, I thought, "well this might end in sweat and tears", because it was very ambitious - hierarchical data, _HeterogenousMetaEstimator
which is not easy to inherit from, the issue with "data must fit the parameters" which I also didn't fully see how to solve, a docstring which is difficult to write with formal accuracy, etcetera.
But none of that blocked you!
Absolutely impressive!!
Welcome to sktime, @VyomkeshVyas!
Way to make an entrance.
Well, that's why |
HierarchyEnsembleForecaster
for panel/hierarchical dataHierarchyEnsembleForecaster
for level- or node-wise application of forecasters on panel/hierarchical data
Reference Issues/PRs
Fixes #2764
What does this implement/fix? Explain your changes.
HierarchyEnsembleForecaster() aggregates panel-type data and applies different univariate forecasters on the aggregated data by each hierarchical level/node. For aggregation, it employs sktime's bulit-in 'Aggregator' class.
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
A reviewer should concentrate their feedback on the forecaster's ability to :-
Any other comments?
PR checklist
For all contributions
For new estimators