Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1341 +/- ##
===========================================
+ Coverage 90.66% 90.69% +0.03%
===========================================
Files 134 135 +1
Lines 14455 14503 +48
===========================================
+ Hits 13105 13154 +49
+ Misses 1350 1349 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@garrettwrong Ok, thankfully I was able to recover this branch. The changes in response to your initial review of the original PR #1322 start at commit 4257cbf labeled "docstring" in the commit history above. These are all changes we discussed in the review comments in #1322 which you have resolved (presumably after looking at the changes). At any rate, this is ready for you to take another look. Let me know if you have any questions that will save you time on the review.
|
|
Glad you were able to find the work. Please reset/squash these down so there is less risk of corrupting the repo history or bringing in regressions. We'll need to re-review them like a new PR because of the history mix up, but it should be pretty smooth. Thanks |
j-c-c
force-pushed
the
cl_reorg_recovery
branch
3 times, most recently
from
f752a79 to
44d405e
Compare
|
Hey, just double checking, this is ready to review? I can get started with it while my other task is running in bg... Thanks |
Yep this is ready. Just finished looking it over and updating the branch. Thanks! |
garrettwrong
left a comment
garrettwrong
left a comment
There was a problem hiding this comment.
Compared with the class document I had from a long time ago. Everything looks in place for this stage of the reorg.
I have 3 suggestions about the same sort of code. Its not new code, just migrated. Maybe it can be improved (maybe not). Make a followup issue to review those in the future after the reorg stuff. Low priority, just something I might have missed before. Thanks
|
|
||
| Rj_tilde = Ri_tildes[j] | ||
| # Generate shifted versions of images. | ||
| pf_i_shifted = np.array([pf_i * shift_phase for shift_phase in shift_phases]) |
There was a problem hiding this comment.
can we just multiply these arrays....
| ] | ||
| ) | ||
| # Compute all possible rotations between the i'th and j'th images. | ||
| Us = np.array( |
There was a problem hiding this comment.
Similarly here, seems like a broadcast would work? This might be a rare case where einsum could be useful.
| c2s = _cl_angles_to_ind(c2s, n_theta) | ||
|
|
||
| # Perform correlation, corrs is shape n_shifts x len(theta_ijs). | ||
| corrs = np.array( |
There was a problem hiding this comment.
This would probably be clearer without the nested list comprehension. And you might not need to reshape it ....
|
@garrettwrong Great, thanks for the review!
I created issue #1345 and will follow up after the reorg work. Thanks |
j-c-c
force-pushed
the
cl_reorg_recovery
branch
from
44d405e to
d90bc13
Compare
3 participants
Recovering the previously reviewed changes from #1322 that I mistakenly rebased over with the wrong version of code prior to merging into develop.
Setting this as a draft while I carefully review that it includes all of the correct changes.