Skip to content
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

Add support for grouped time series #51

Merged
merged 6 commits into from Feb 23, 2021

Conversation

noahsa
Copy link
Contributor

@noahsa noahsa commented Jan 28, 2021

Added the capability to create a grouped time series summing matrix. The methodology takes a slightly different string-based approach to build the summing matrix by string searching the node's keys within the tree, based on the assumption that nodes keys are delimited by underscores(could be any character) and the levels in the hierarchy are unique. I would love your feedback or any further discussion.

If this is a duplicate or already possible in the library, please let me know and I'd be happy to update the documentation to help others like myself with some guidance.

I have included unit tests.

I have not updated the documentation, since it will need to be updated in a couple of places/ examples. Although, I am happy to make the updates based on feedback.

@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #51 (d4855c9) into master (6e8a257) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   83.82%   83.93%   +0.11%     
==========================================
  Files          18       18              
  Lines         847      853       +6     
==========================================
+ Hits          710      716       +6     
  Misses        137      137              
Impacted Files Coverage Δ
hts/convenience.py 94.11% <100.00%> (ø)
hts/core/regressor.py 83.00% <100.00%> (ø)
hts/functions.py 96.22% <100.00%> (+0.03%) ⬆️
hts/utilities/load_data.py 87.03% <100.00%> (+1.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e8a257...d4855c9. Read the comment docs.

@carlomazzaferro
Copy link
Owner

Sorry for the late one, but this looks really good to me. Thanks for taking the time! I'll approve for now, but I'd like to get the documentation updated in the time being. Would you be open to that?

@noahsa
Copy link
Contributor Author

noahsa commented Feb 7, 2021

Yes, I'd be happy to! I'll work on that this week.

@noahsa
Copy link
Contributor Author

noahsa commented Feb 8, 2021

All documentation/ unit tests are updated.

Two additional change made:

  1. Returned the labels associated with the rows of the summing matrix from to_sum_mat as well.
  2. Added an example to the docs of OLS optimal reconciliation using forecasts created outside scikit-hts.

I did not update HISTORY.rst, since it looks to be tied to bumping the version. I'm new to this, so I was not if I should do this.

Please let me know if there is anything that I missed.

@carlomazzaferro
Copy link
Owner

This is outstanding work, documentation also looks good to me. The only thing I'm slightly worried is the case in which people have other columns with underscores -- but the documentation makes it clear enough what the structure should be. Merging in! Thanks for working on this

@carlomazzaferro carlomazzaferro merged commit 3878608 into carlomazzaferro:master Feb 23, 2021
@noahsa
Copy link
Contributor Author

noahsa commented Feb 23, 2021

Thank you! I agree, the underscore constraint is bit worrisome. It would be fairly easy to allow the user to specify a delimiter other than underscore, but more thought would need to go into eliminating the need for a delimiter all together in a future PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants