Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1391 +/- ##
===========================================
+ Coverage 88.18% 88.83% +0.65%
===========================================
Files 135 136 +1
Lines 14927 15824 +897
===========================================
+ Hits 13164 14058 +894
- Misses 1763 1766 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
28ba09b to
afd1d2b
Compare
garrettwrong
left a comment
garrettwrong
left a comment
There was a problem hiding this comment.
A few easy suggestions in-line but I do have larger concerns about the NUG code and am kicking it back so you can address them.
i) We don't appear to include a reproduction of original NUG for C1. If Amit is looking for complete coverage of groups' methods, this is required.
ii) We don't appear to include the symmetric extension authors reproducible example experiment. This is important since the method did not appear to work on the best experimental data we have available, so we will need this reproducible example as the full scientific test. The data will need to be made available for this purpose.
iii) After about line 300 or so into nug, where I oof'ed, the code becomes difficult to read/follow, with little documentation of the (many) variables. I had to start backing off the detailed review at that point, unfortunately. Not trying to reverse engineer that thing. While some docstrings are good, many are hit/miss, and missing coverage of their args/returns. Given (iv), this is sort of important. I didn't want to annotate every miss, too much.
iv) There are places where there are an excess of variables that could/should probably be restructured. This is normally the type of work the RSE's do. If we aren't restructuring the code to be improved (in some cases not merited/effective, I get it), I would expect the documentation to be better...
v) A lot of the xp code looks questionable. I have concerns that np was replaced with xp without considering the operations enough. Creating/performing many small array operations can often perform worse and is a poor use of limited resources. Even if performance is at par, if it fragments/uses GPU memory; this implementation may negatively impact later components that want to use the GPU for gains. I would want to see some evidence as to which of those areas actually effectively use the GPU and places that don't be reverted. Perhaps I'm totally misunderstanding some of the sizes/operations there... Some of the ones that look like might offer more were not converted, so I found it generally confusing.
vi) It is sort of confusing which methods were chosen to be static, which were not, and how they are nested (or not).
vii) a lot of the code has strange indexing. In some cases it would actually appear to be simpler as 0-based, but is written (like a port of) 1-based. I recognize a few of the areas might be cleaner as 1-based, and many neutral but are written 1-based. This is odd; reads like the code started in MATLAB and passed through multiple people who didn't clean up the indexing...
vii) I'm not sure how performance critical we need to be about this code, but the way it is written in some places has a lot of inefficient implementations. Lots of lists, scalars, lists of small arrays etc being constructed and passed around. Because of how the code is written, it isn't immediately clear where the bulk of compute is happening, so I can't offer much more there based on my limited review. (or if it matters...)
viii) The printing formatting, while fine, are not consistent with the rest of ASPIRE. In this file, there are multiple types of formatting. Probably better to convert to f-strings. FWIW, if it was not part of larger code base that uses the other formatting I would have no problem with the C style printf formats. Having multiple styles is not ideal.
I think there were some other things but I'm running out of steam/time. This is plenty. Some of those need to be addressed. Others, well, I'm not sure to what extent you've already improved from Ruiyi's code, or if the extra effort is worth it for this algorithm's results. We can discuss the right level of effort for this algo with Joakim and/or Amit when I get back, but you should have enough to stay busy until then.
| return request.param | ||
|
|
||
|
|
||
| @pytest.fixture(params=PR, ids=lambda x: f"proximal_refine={x}", scope="module") |
There was a problem hiding this comment.
whats the purpose of this fixture?
| Initialize the symmetric NUG orientation estimator. All default values match those used | ||
| for publication results, with exception of `pr_iters`, which controls how many iterations | ||
| of proximal refinement to be used. The default of None runs the algorithm without proximal | ||
| refinement. Set `pr_iters=4` to match t3he proximal refinement workflow in the related |
| Initialize the symmetric NUG orientation estimator. All default values match those used | ||
| for publication results, with exception of `pr_iters`, which controls how many iterations | ||
| of proximal refinement to be used. The default of None runs the algorithm without proximal | ||
| refinement. Set `pr_iters=4` to match t3he proximal refinement workflow in the related |
There was a problem hiding this comment.
Why is the PR not on by default? is it covered in the unit tests?
|
|
||
| Ii_hat = self.pf_full[i] | ||
| Ij_hat = self.pf_full[j] | ||
| idxi = np.round((alpha - np.pi / 2) * n_theta / 2 / np.pi) % n_theta |
There was a problem hiding this comment.
maybe for cases like this, make it an integer directly if you don't intend to use idx as floats
| """ | ||
| dk = 2 * k + 1 | ||
|
|
||
| exp_alpha_grid = np.zeros((2 * T, dk), dtype=complex_type(np.float64)) |
There was a problem hiding this comment.
no need for complex_type() of a static types
|
|
||
| self.C = C | ||
|
|
||
| def complex2real(self, ell): |
There was a problem hiding this comment.
does this require anything from self?
|
|
||
| # Solve the symmetry-constrained SDP relaxation in the real, packed | ||
| # representation basis prepared by ADMM_preprocessing. | ||
| ( |
| P = [] | ||
| for k in range(1, Lmax + 1): | ||
| dk = 2 * k + 1 | ||
| Pk = xp.eye(dk, dtype=np.float64) |
There was a problem hiding this comment.
not clear to me it is useful to put so many small matrices on GPU... are they involved in something heavy later?
| ) | ||
| self.X_est = X_est | ||
|
|
||
| def admm_sym_J(self, C, verbose): |
There was a problem hiding this comment.
did you check if having all these small matrices on the GPU was faster?... it may not be worth the overhead...
| # Allocate one coefficient matrix per Wigner degree; | ||
| # each is block-indexed by image pair. | ||
| C = [] | ||
| for k in range(1, Lmax + 1): |
There was a problem hiding this comment.
if it makes more sense for k (L) to be zero based, it looks pretty easy to convert all the uses by changing the dk line.
Similar concerns throughout the file.
2 participants
Adds the port of Ruiyi's NUG algorithm that handles Cn/Dn symmetric molecules.
As of now, most variables names have been left as in the original code. Many methods have been optimized via broadcasting/vectorization. GPU logic is now handled via the
xpnamespace. Other code alterations were made to fit within our orientation estimation framework and class structure. Additional changes were made to leverage in-house methods such asPolarFT, Handling non-zero offset images, and symmetry parsing.