Skip to content

Conversation

@j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Aug 9, 2023

This PR consists of

  • Refactoring PolarBasis2D
    • Use only half of the polar Fourier rays everywhere
    • Change to PolarFT
    • Replace hardcoded tests with logical based tests
  • Refactor commonline_base and commonline_sync code to work with refactored pft
    • Use rays from [0, n_theta / 2) instead of [n_theta / 2, n_theta)
    • Ensure all methods are following the new pf convention
    • Replace hardcoded tests with parametrized logical based tests

Note, this PR is to address some issues encountered in a downstream branch for the port of the common-lines method using SDP. It was found that our convention for building the common-lines matrix was causing the SDP method to fail.

@j-c-c j-c-c added bug Something isn't working cleanup labels Aug 9, 2023
@j-c-c j-c-c self-assigned this Aug 9, 2023
@j-c-c j-c-c marked this pull request as draft August 9, 2023 18:24
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #994 (507c337) into develop (5d6f66b) will increase coverage by 0.02%.
The diff coverage is 98.78%.

@@             Coverage Diff             @@
##           develop     #994      +/-   ##
===========================================
+ Coverage    88.79%   88.82%   +0.02%     
===========================================
  Files          127      127              
  Lines        11321    11359      +38     
===========================================
+ Hits         10053    10090      +37     
- Misses        1268     1269       +1     
Files Changed Coverage Δ
src/aspire/basis/__init__.py 100.00% <ø> (ø)
src/aspire/utils/__init__.py 100.00% <ø> (ø)
src/aspire/utils/rotation.py 97.66% <95.45%> (-0.42%) ⬇️
src/aspire/abinitio/commonline_base.py 94.87% <100.00%> (ø)
src/aspire/operators/__init__.py 100.00% <100.00%> (ø)
src/aspire/operators/polar_ft.py 100.00% <100.00%> (ø)
src/aspire/utils/coor_trans.py 92.71% <100.00%> (+1.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@j-c-c j-c-c mentioned this pull request Aug 10, 2023
@garrettwrong
Copy link
Collaborator

Is this guy ready for review? It looks like it was auto triggered, but if your are still working on it I won't put on my todo list quite yet. Thanks

@j-c-c
Copy link
Collaborator Author

j-c-c commented Aug 15, 2023

Is this guy ready for review? It looks like it was auto triggered, but if your are still working on it I won't put on my todo list quite yet. Thanks

Not quite ready. I mistakenly created this as PR instead of a draft. I reverted to a draft, but it looks like you guys will still get pinged about it.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Aug 18, 2023

Hey @garrettwrong, this is ready for an initial review. I'm still working on fixing up the estimate_shifts method. I might be able to squeeze it into this PR, but wanted to get an initial review since I'll be out for a few days.

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.

Hey, this is great. Thank you for taking the time to clean this up and verify the implementation.

Mostly quick small stuff. There is the one mode testing issue, but I took a look at it and hope the suggestion is helpful.

@j-c-c
Copy link
Collaborator Author

j-c-c commented Aug 24, 2023

Oops, sorry @garrettwrong. Left a breakpoint() in there. Should be all good now.

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.

Still a couple stragglers, but otherwise looking good. Thanks.

garrettwrong
garrettwrong previously approved these changes Aug 25, 2023
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.

🚀

@j-c-c j-c-c marked this pull request as ready for review August 25, 2023 13:22
@garrettwrong garrettwrong self-requested a review August 25, 2023 13:56
garrettwrong
garrettwrong previously approved these changes Aug 25, 2023
@j-c-c
Copy link
Collaborator Author

j-c-c commented Aug 25, 2023

@janden, this is ready for you to take a look whenever you get a chance. Tagging you here since I mistakenly opened this as a PR instead of a draft and you might have not gotten a review request when I marked as "Ready for Review".

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.

Looks good! Just a few things.

@j-c-c j-c-c requested a review from janden August 29, 2023 13:26
@j-c-c j-c-c merged commit 82763cf into develop Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants