Nixtla / hierarchicalforecast

Probabilistic Hierarchical forecasting 👑 with statistical and econometric methods.
https://nixtlaverse.nixtla.io/hierarchicalforecast
Apache License 2.0
588 stars 76 forks source link

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

Closed elephaint closed 1 month ago

elephaint commented 1 month ago
review-notebook-app[bot] commented 1 month ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

elephaint commented 1 month ago

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.