Add generalized eigenvalue decomposition, fix some bugs #39
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Overall looks really good to me, and definitely in the same style as the rest so I think you nailed it!
I like the generalization of the macro to multiple arguments. Given that we only have a single keyword supported and this is mostly an internal thing anyways, I would even be fine with dropping the specification altogether: @functiondef 2 f seems also good to me. This ultimately doesn't really matter though so I have no strong opinions here.
As an alternative, an idea that might also work is to simply bundle the arguments together: gen_eig!((A, B), ...) might actually require less work for altering the rest of the functionality, but I'm not sure if it is that much more convenient, nor easy to remember/read etc. Just wanted to share the idea, definitely feel free to let me know what you think about it because I'm not convinced it is better.
For the distinction between gen_eig and gen_eig_full, I assume this is because for eig and eig_full we have a similar function laying around somewhere. I think this is an idea that made it in at some point and was never properly worked out, I'm not sure if we really need both (I would personally prefer if we could get rid of one of them even).
For eig, we only export the eig_full variants, so I would say to do the same here, and at some point just remove eig etc altogether?
| end | ||
| end | ||
|
|
||
| function _reorder_realeigendecomposition!(W, WR, WI, Wbeta, work, VR, jobvr) |
There was a problem hiding this comment.
Possibly can use inbounds annotations in this function, not sure if this matters?
There was a problem hiding this comment.
It might for large arrays
Indeed I think the same result might be achievable with a little more type specification on the arguments and making them a |
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Nice! I would prefer to use the ! for the gaugefix function because it is in-place, otherwise good to go for me!
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Tried this as a way to learn about the structure of the package, so please feel free to aggressively critique it :).
I've added supported for generalized eigendecomposition over two inputs,
AandB. All this arose out of a desire to add coverage to theggevmethods. I fixed some errors in theccallwrapper and tried to implement the interface in a semi-reasonable way. Annoyingly, LAPACK doesn't provide the support forLAPACK_Expertfor this...