Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

D2 Common-lines Algorithm #1116

Merged
merged 105 commits into from
Sep 20, 2024
Merged

D2 Common-lines Algorithm #1116

merged 105 commits into from
Sep 20, 2024

Conversation

j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented May 14, 2024

Port of the D2 common-lines algorithm for orientation estimation.

@j-c-c j-c-c added the enhancement New feature or request label May 14, 2024
@j-c-c j-c-c self-assigned this May 14, 2024
@j-c-c j-c-c force-pushed the D2_cl branch 2 times, most recently from 265394c to becf281 Compare June 5, 2024 18:57
@j-c-c j-c-c force-pushed the D2_cl branch 2 times, most recently from 09c400d to 86cf378 Compare June 17, 2024 17:17
@j-c-c j-c-c requested a review from garrettwrong June 17, 2024 18:23
garrettwrong
garrettwrong requested changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. I understand you had success validating against MATLAB, which is great. This looks like a rough but workable initial python port. There is still quite a bit of code cleanup to do here. I highlighted places that stuck out to me. Because of the verbosity of the code and the amount of changes needed I didn't really dig in too deeply yet.

My advice would be to codify any additional (repro) tests you have, even if it isn't part of the PR, then begin cleaning up the minor/benign/reshape sort of changes before moving on to the more complicated broadcasts... checking you still repro along the way. I think you will find the volume of code can be reduced quite a bit. Let me know if you have any questions about the vectorizing hints.

src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
tests/test_orient_d2.py Show resolved Hide resolved
tests/test_orient_d2.py Show resolved Hide resolved
tests/test_orient_d2.py Show resolved Hide resolved
tests/test_orient_d2.py Outdated Show resolved Hide resolved
tests/test_orient_d2.py Show resolved Hide resolved
@j-c-c
Copy link
Collaborator Author

j-c-c commented Aug 27, 2024

@garrettwrong this is ready for you to take another look. I've addressed all of your comments. There are a couple changes that I've refrained from making to maintain exact outputs for this first set of changes (I've noted where this is the case).

@garrettwrong
Copy link
Collaborator

Awesome! I'll start working on it next after I get the sync3n back to Joakim.

garrettwrong
garrettwrong requested changes Sep 5, 2024
View reviewed changes
Copy link
Collaborator

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, just couple stragglers and a few items that are marked to fix at the end of the review process.

I'm guessing you want to go through the Joakim review process before making changes that might cause differences from the MATLAB repro?

@j-c-c
Copy link
Collaborator Author

j-c-c commented Sep 5, 2024

@garrettwrong I reverted the search radius to r=2 since it was breaking a test. I'll have to take a closer look after lunch

@j-c-c
Copy link
Collaborator Author

j-c-c commented Sep 10, 2024

@garrettwrong I reverted the search radius to r=2 since it was breaking a test. I'll have to take a closer look after lunch

Turns out it wasn't this search radius breaking the test. Put this back to what we discussed, ie. np.ceil(2 * n_theta / 360).

garrettwrong
garrettwrong previously approved these changes Sep 11, 2024
View reviewed changes
@j-c-c j-c-c marked this pull request as ready for review September 11, 2024 14:07
@j-c-c j-c-c requested a review from janden as a code owner September 11, 2024 14:07
janden
janden requested changes Sep 12, 2024
View reviewed changes
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congratulations on putting this together! I can't say that I'm able to follow all the details, but here are a couple of questions and comments. Nothing big here.

src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Outdated Show resolved Hide resolved
src/aspire/abinitio/commonline_d2.py Show resolved Hide resolved
tests/test_orient_d2.py Outdated Show resolved Hide resolved
tests/test_orient_d2.py Outdated Show resolved Hide resolved
janden
janden previously approved these changes Sep 19, 2024
View reviewed changes
@j-c-c
randomize color sync test
0f025af
@j-c-c
Add documentation to color_sync test.
0551654
@j-c-c
remove F-order flatten
4f05efa
@j-c-c
Reword docstring
18afca9
@j-c-c
Add docstrings
7502450
@j-c-c
Use number rays in 2 degrees instead 2 pf rays in all_eq_measures.
7e1d66e
@j-c-c
Add comment about necessary ordering of D2 symmetry group elements.
400c5ec
@j-c-c
black
a331001
@j-c-c
revert all_eq_measures search radius.
9181d41
@j-c-c
black
8cf8bf9
@j-c-c
Add seed to sporadically failing test.
7aeaf82
@j-c-c
revert search radius
e61695a
@j-c-c
typo
a5bc37d
@j-c-c
Use np.nonzero instead of np.where.
7bab705
@j-c-c
lowercase
fcf92fa
@j-c-c
use ints
b3c6bed
@j-c-c
input/output docs
b20e1e0
@j-c-c
break up _sync_signs
e8c2221
@j-c-c
Rewrite docstring
e6cc527
@j-c-c
resolve numpy deprecation warning.
d4a0547
@j-c-c
try diff seed
2014b42
@j-c-c
switch test to doubles to diagnose osx-arm failures.
fcf1567
@j-c-c
Remove eq_idx
84048e0
@j-c-c
Add descript of eq2eq table to docs.
357e5d3
@j-c-c
add more documentation.
b948d7a
@j-c-c
Always doubles for scipy LinearOperator
f30c560
@j-c-c
revert test to singles
ae9b98f
@j-c-c j-c-c merged commit f31d317 into develop Sep 20, 2024
33 checks passed
@garrettwrong
Copy link
Collaborator

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garrettwrong garrettwrong garrettwrong left review comments

@janden janden janden left review comments

Assignees

@j-c-c j-c-c

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@j-c-c @garrettwrong @janden