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

[FEAT] Improve residuals-based reconciliation stability and faster ma.cov #295

Merged
merged 10 commits into from
Oct 17, 2024

Conversation

elephaint
Copy link
Contributor

@elephaint elephaint commented Oct 10, 2024

  • Adds faster ma.cov method to mint_cov
  • Improves stability of residuals-based methods.
  • Includes test for covariance matrices in MinT
  • Includes benchmark that can be run for MinT and ma.cov, useful for future benchmarking

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@elephaint elephaint changed the title [FIX] Improve MinT stability and faster ma.cov [FIX] Improve residuals-based reconciliation stability and faster ma.cov Oct 10, 2024
Copy link
Contributor

@christophertitchen christophertitchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few suggestions—good stuff!

hierarchicalforecast/methods.py Show resolved Hide resolved
hierarchicalforecast/methods.py Outdated Show resolved Hide resolved
Copy link
Contributor

@christophertitchen christophertitchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was so confused why you were transposing a square zero matrix but it makes complete sense now that you wanted to change the order! 😅

What are you using to benchmark there? It looks amazing and much nicer than timeit. Yes, if you want the output $W$ to have 0 instead of np.nan, your approach is definitely the best. I am surprised that the strict typing had any impact on the performance in this case, even when being really pedantic and specifying a row-major $W$ return with f8[::1, :].

Anyway, the last thing I can think of would be to specify the Fortran order when allocating and initialising the array, but transpose is so fast as it only needs to swap the strides of the array, so it would probably only shave off a few nanoseconds or microseconds depending on the number of time series.

    W = np.zeros((n_timeseries, n_timeseries), dtype=np.float64, order="F")

LGTM! 🚀

@elephaint
Copy link
Contributor Author

elephaint commented Oct 15, 2024

I was so confused why you were transposing a square zero matrix but it makes complete sense now that you wanted to change the order! 😅

What are you using to benchmark there? It looks amazing and much nicer than timeit. Yes, if you want the output W to have 0 instead of np.nan, your approach is definitely the best. I am surprised that the strict typing had any impact on the performance in this case, even when being really pedantic and specifying a row-major W return with f8[::1, :].

Anyway, the last thing I can think of would be to specify the Fortran order when allocating and initialising the array, but transpose is so fast as it only needs to swap the strides of the array, so it would probably only shave off a few nanoseconds or microseconds depending on the number of time series.

    W = np.zeros((n_timeseries, n_timeseries), dtype=np.float64, order="F")

LGTM! 🚀

Thanks - I agree. Still not sure why static typing makes things worse, it must be because there's a contiguity mismatch or something, but I think tried all the right combinations.

The reason the .T is necessary is that Numba doesn't support the order-argument of Numpy functions (that's why I wrote it like that, it's a bit of a low-cost hack to get F-order in Numba).

The testing is done with pytest-benchmark - I've added the tests in a folder tests. You can run them using the following command (make sure to pip install pytest pytest-benchmark) to run the covariance benchmark:

pytest tests\test_benchmark.py::test_cov -v -s --benchmark-min-rounds=20

I then make changes, nbdev_export them and run the benchmark again, usually twice and/or also cleaning cache manually to make sure there's no caching shenanigans going on.

@elephaint elephaint changed the title [FIX] Improve residuals-based reconciliation stability and faster ma.cov [FEAT] Improve residuals-based reconciliation stability and faster ma.cov Oct 15, 2024
@elephaint elephaint merged commit a9a5866 into main Oct 17, 2024
17 checks passed
@elephaint elephaint deleted the fix/improve_mint_stability branch October 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants