-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
elephaint
commented
Oct 10, 2024
•
edited
Loading
edited
- 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
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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 left a few suggestions—good stuff!
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 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 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 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 The testing is done with
I then make changes, |