Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## Jsync_gpu #1364 +/- ##
==========================================
Coverage 90.84% 90.84%
==========================================
Files 135 135
Lines 14658 14658
==========================================
Hits 13316 13316
Misses 1342 1342 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
j-c-c
changed the title
garrettwrong
left a comment
garrettwrong
left a comment
There was a problem hiding this comment.
Thanks! Overall this is great. Couple minor changes, and a couple questions.
There is one lingering question we can discuss in the meeting, and that is how detailed the docs should be.
| src: ImageSource | ||
| +estimate_rotations() | ||
| +estimate_shifts() | ||
| +pf |
There was a problem hiding this comment.
is pf a method or attribute or?
what about the attributes for rots/shifts? (and maybe other things too..?, hrmm how detailed should our detailed docs be)
| The relationships below show how the concrete solvers inherit shared behaviors (masking, CL matrix | ||
| utilities, synchronization helpers) before layering on algorithm-specific logic. | ||
|
|
||
| .. mermaid:: |
There was a problem hiding this comment.
This looks great! Yay 🙏
I have a suspicion something is missing, but I might just be mixing things up. The diagrams I made for you before are in the office. Did you leave anything out on purpose?
| synchronization step is not needed. An eigen-decomposition is then performed to recover the image | ||
| orientations. | ||
| - ``CommonlineSDP`` (:file:`src/aspire/abinitio/commonline_sdp.py`): This method uses a | ||
| relaxation of the least squares formulation of the orientation problem and frames it |
There was a problem hiding this comment.
reread this one; probably want how not you worded.
| scaffolding but substitutes an ADMM-based least unsquared deviations solver. Parameters | ||
| like ``alpha``, ``mu`` scheduling, and adaptive rank selection govern convergence. | ||
| - ``CommonlineIRLS`` (:file:`src/aspire/abinitio/commonline_irls.py`): Wraps LUD inside | ||
| an outer reweighting loop, updating residual weights and ``self._mu`` to improve |
There was a problem hiding this comment.
I wonder more than ever if _mu was the right name there lol
There was a problem hiding this comment.
Yeah. Actually, was meaning to take this out of here in favor of just the description. Missed it on my last pass.
There was a problem hiding this comment.
No worries, it is probably more of a code change than this doc PR. If it isn't there, then wont need to update it later 😇 .
| clmatrix = self.clmatrix | ||
|
|
||
| # Custom synchronization using clmatrix statistics | ||
| sync_mat = self._fancy_sync(clmatrix) |
There was a problem hiding this comment.
While I tried not to be too much of a pain about it in the refactors... I'm pretty sure we should not be suggesting this practice of passing and returning variables instead of assignment in our "this is how to make a new class" code section 😂 . (With the exception of methods that will be iterative etc, like that _mu thing above, which ironically, we do assign 😂 😂 ).
Perhaps use attributes instead of args. Extra credit if you want to fix our actual code too.
If you disagree, no worries, but at least meet me in the middle and assign them to attibributes in estimate_rotations. Then the poor future developer (likely you) can look at them without manually rerunning half the method line-by-line or being forced to alter the code... it will make things easier when you get the emails like how do I get the sync matrix like in this MATLAB function etc. I have had to do that exact thing at least once a year for 6 years now.
2 participants
Adds a doc page for a high-level overview of the CL methods