Skip to content

FLE Basis #693

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.

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

Merged
merged 146 commits into from
Feb 17, 2023
Merged

FLE Basis #693

merged 146 commits into from
Feb 17, 2023

Conversation

chris-langfield
Copy link
Collaborator

@chris-langfield chris-langfield commented Sep 9, 2022

Porting https://github.com/nmarshallf/fle_2d/ into ASPIRE.

Old version:
Relevant paper: https://arxiv.org/pdf/2207.13674.pdf

@chris-langfield chris-langfield self-assigned this Sep 9, 2022
@codecov
Copy link

codecov bot commented Sep 12, 2022

Codecov Report

Merging #693 (3c22ce8) into develop (a30567c) will increase coverage by 0.89%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #693      +/-   ##
===========================================
+ Coverage    88.33%   89.22%   +0.89%     
===========================================
  Files          113      115       +2     
  Lines         8922     9813     +891     
===========================================
+ Hits          7881     8756     +875     
- Misses        1041     1057      +16     
Impacted Files Coverage Δ
src/aspire/basis/__init__.py 100.00% <100.00%> (ø)
src/aspire/basis/fle_2d.py 100.00% <100.00%> (ø)
src/aspire/basis/fle_2d_utils.py 100.00% <100.00%> (ø)
src/aspire/basis/steerable.py 88.77% <100.00%> (ø)
src/aspire/nufft/__init__.py 82.41% <100.00%> (ø)
src/aspire/utils/relion_interop.py 93.65% <0.00%> (-6.35%) ⬇️
src/aspire/source/image.py 97.22% <0.00%> (-0.25%) ⬇️
src/aspire/source/relion.py 96.83% <0.00%> (-0.08%) ⬇️
src/aspire/utils/__init__.py 100.00% <0.00%> (ø)
src/aspire/source/coordinates.py 93.65% <0.00%> (+1.21%) ⬆️
... and 1 more

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

@chris-langfield
Copy link
Collaborator Author

Turns out the fle_ells_sign method wasn't actually needed (holdover from previous method of getting FB ordering).

Think we addressed the main comments from this review. A few unresolved ones will be useful for future developers.

@chris-langfield chris-langfield marked this pull request as ready for review February 14, 2023 20:36
@chris-langfield
Copy link
Collaborator Author

Grid issue will be up by EOD of tomorrow.

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.

One last quick question regarding the sign flipping order thing. Otherwise anything unresolved is probably going to stay that way until we can take another closer pass.

I still have concerns with the code (grid, correspondence, and now the CTF convolution 😬 too). Will catch up with you later about the issue documentation. We can see what Joakim thinks about next steps (merging) tomorrow at our meeting.

Thanks for all your effort tidying this up!

):
_final_num_basis_functions -= 1

# potentially subtract one to keep complex conjugate pairs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. I'm going to leave this conversation open for the future, but nothing to do now.

@chris-langfield
Copy link
Collaborator Author

Thanks Garrett, sounds good!

garrettwrong
garrettwrong previously approved these changes Feb 15, 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.

I think this is about as far as we can take this with the time left. Lets discuss it more tomorrow as a group. Thanks!

@chris-langfield
Copy link
Collaborator Author

Created #856 to track the odd resolutions grids problem

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.

Unless there were outstanding requests from Joakim that I missed, I think this ready to merge.

If we want to look at updating any issues, can make another PR.

@chris-langfield chris-langfield merged commit 2e01925 into develop Feb 17, 2023
@garrettwrong garrettwrong deleted the fle_basis branch June 1, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Optimization Performance or Resource Optimzation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FBBasis2D and FLEBasis2D correspondence
4 participants