Block Diagonal Class #144

garrettwrong · 2020-04-15T14:02:58Z

For my first pull request with this group, I started with something a manageable size.

A BlockDiagonal class replaces blk_diag_* functions. This includes:

operator overloading, and some additional operations
slightly optimized in-place operations
some additional unit tests
Replace known uses of blk_diag

In the future, it is possible to optimize this code several ways. One way is to parallelize the block computations in python. Alternatively, the code was tested and is able to be Cython-ized, but this requires some ugly adjustments. Such optimizations should be easier now that we are consistently using the class (afaict).

There are still a few todo items I will complete, and I would like to review the code myself with fresh eyes as well, but I think it is close enough that we can go through the motions of a review.

Closes #111

tests/test_BlockDiagonal.py

junchaoxia

Overall it looks good. Some discussions can be done to make some changes before merging.

setup.py

src/aspire/estimation/covar2d.py

src/aspire/utils/BlockDiagonal.pyx

src/aspire/basis/__init__.py

garrettwrong · 2020-04-16T19:27:24Z

@junchaoxia I think most of the comments have been addressed if you want to take another peek at the latest. I tried to stack the commits closer to how you discussed as well. Thanks.

garrettwrong · 2020-04-16T19:43:25Z

~~rebased from latest develop that has the pipeline fixes.~~ Perhaps not. Will look tomorrow.

src/aspire/utils/blk_diag_matrix.py

janden

Some things to discuss here. Overall looks very good.

One thing to note: the PR is quite large, which makes reviewing and discussing more difficult (rule of thumb we try to use is roughly one day of work or 800 lines). Large PRs are of course unavoidable sometimes (many moving parts that need to be changed to avoid breaking things). For this type of feature, it might be good to have one PR to add the basic functionality (say without operator overloading), another to extend that functionality, and then another where the functionality is integrated into the rest of the project and the old code is removed.

src/aspire/basis/__init__.py

src/aspire/denoising/denoiser_cov2d.py

src/aspire/estimation/covar2d.py

tests/test_BlkDiagMatrix.py

mostly docstring formatting and readability some minor code improvements

garrettwrong · 2020-04-24T20:00:13Z

@janden I got the first batch of easy stuff. I will go back to the remaining items Monday. I have a better idea what you are looking for regarding style etc now. I got a little rusty at PPPL :). Have a nice weekend.

…x tests

also cleanup whitespace since I was changing doctrings

src/aspire/utils/cell.py

… BlkDiagMatrix

janden

Looks good to me. Just one or two small things.

src/aspire/estimation/covar2d.py

src/aspire/utils/blk_diag_matrix.py

tests/test_BlkDiagMatrix.py

garrettwrong

Following our discussion I implemented the suggested changes. Will merge in once CI pipeline completes.

also cleanup some pep8 whitespace complaints

garrettwrong · 2020-04-30T21:41:50Z

@junchaoxia When you get a second can you review/accept? This way I can avoid using admin overide to merge. Thanks

garrettwrong requested a review from junchaoxia April 15, 2020 14:03

garrettwrong commented Apr 15, 2020

View reviewed changes

tests/test_BlockDiagonal.py Outdated Show resolved Hide resolved

junchaoxia suggested changes Apr 16, 2020

View reviewed changes