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

OrientedSource #890

Merged
merged 85 commits into from
Jun 26, 2023
Merged

OrientedSource #890

merged 85 commits into from
Jun 26, 2023

Conversation

j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Mar 22, 2023 •
edited
Loading

Adds OrientedSource class that consumes a src and an orientation estimation instance (ie. CLSyncVoting) and serves up images with rotations assigned.

Adds a SymmetryGroup class to be used by SyntheticVolume and ImageSource. Instances of ImageSource now have a symmetry_group attribute (which is a SymmetryGroup object) and corresponding metadata field _rlnSymmetryGroup.

See issue #875

@j-c-c j-c-c added the enhancement New feature or request label Mar 22, 2023
@j-c-c j-c-c self-assigned this Mar 22, 2023
@codecov
Copy link

codecov bot commented Mar 22, 2023 •
edited
Loading

Codecov Report

Merging #890 (a16e340) into develop (3b2477c) will increase coverage by 0.18%.
The diff coverage is 98.58%.

@@             Coverage Diff             @@
##           develop     #890      +/-   ##
===========================================
+ Coverage    88.51%   88.70%   +0.18%     
===========================================
  Files          120      120              
  Lines        10445    10585     +140     
===========================================
+ Hits          9245     9389     +144     
+ Misses        1200     1196       -4
Impacted Files Coverage Δ
src/aspire/abinitio/__init__.py 100.00% <ø> (ø)
src/aspire/abinitio/commonline_base.py 94.79% <ø> (-0.09%) ⬇️
src/aspire/source/relion.py 97.97% <ø> (+1.01%) ⬆️
src/aspire/source/image.py 92.10% <96.00%> (+0.73%) ⬆️
src/aspire/commands/orient3d.py 100.00% <100.00%> (ø)
src/aspire/source/__init__.py 100.00% <100.00%> (ø)
src/aspire/source/coordinates.py 92.30% <100.00%> (ø)
src/aspire/source/simulation.py 98.97% <100.00%> (+0.01%) ⬆️
src/aspire/volume/__init__.py 100.00% <100.00%> (ø)
src/aspire/volume/symmetry_groups.py 100.00% <100.00%> (ø)
... and 2 more

... and 1 file with indirect coverage changes

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

@j-c-c j-c-c changed the title Symmetry groups OrientedSource Mar 23, 2023
@garrettwrong
Copy link
Collaborator

Moving to v0.11.1

@j-c-c j-c-c requested a review from garrettwrong April 20, 2023 17:39
@j-c-c j-c-c requested review from garrettwrong and removed request for garrettwrong April 28, 2023 17:12
garrettwrong
garrettwrong requested changes May 3, 2023
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.

Solid start, thanks. Mostly minor oversights, but there is also still some refactoring to do. Think the suggestions over and let me know if I got any wrong or you are stuck.

gallery/experiments/experimental_abinitio_pipeline_10073.py Outdated Show resolved Hide resolved
gallery/experiments/simulated_abinitio_pipeline.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Show resolved Hide resolved
tests/test_symmetry_groups.py Outdated Show resolved Hide resolved
tests/test_oriented_source.py Outdated Show resolved Hide resolved
tests/test_oriented_source.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Show resolved Hide resolved
@j-c-c
Copy link
Collaborator Author

j-c-c commented May 22, 2023

@garrettwrong I've done a bit of refactoring and cleanup. I think this is in a good place to take another look. Thanks!

@j-c-c j-c-c requested a review from garrettwrong May 22, 2023 19:45
garrettwrong
garrettwrong requested changes May 24, 2023
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.

Almost there, couple small tweaks on the refactored code. Thanks!

gallery/experiments/experimental_abinitio_pipeline_10073.py Outdated Show resolved Hide resolved
gallery/tutorials/pipeline_demo.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Show resolved Hide resolved
src/aspire/source/image.py Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/volume.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
src/aspire/volume/volume.py Show resolved Hide resolved
@j-c-c j-c-c mentioned this pull request May 24, 2023
Open
@j-c-c j-c-c requested a review from garrettwrong May 24, 2023 20:58
garrettwrong
garrettwrong previously approved these changes May 24, 2023
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 getting those updates through. Looks like just a minor merge conflict to resolve and you should be ready for the next review :).

@j-c-c j-c-c marked this pull request as ready for review May 25, 2023 14:02
@j-c-c j-c-c requested a review from janden as a code owner May 25, 2023 14:02
janden
janden requested changes Jun 12, 2023
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.

Looks good. I've only reviewed core (src/*) code for now. Once that's stable I'll tackle the gallery and tests.

src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Show resolved Hide resolved
src/aspire/volume/volume_synthesis.py Outdated Show resolved Hide resolved
src/aspire/volume/symmetry_groups.py Outdated Show resolved Hide resolved
src/aspire/volume/volume_synthesis.py Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
src/aspire/source/image.py Outdated Show resolved Hide resolved
@j-c-c j-c-c requested a review from janden June 15, 2023 20:01
@j-c-c
Copy link
Collaborator Author

j-c-c commented Jun 15, 2023

@janden I've addressed all of your initial comments/questions, so I think this is in a good place to take another look.

The only unresolved discussions are whether our symmetry groups should be in O(3) or SO(3) and if we want an IdentitySymmetryGroup.

j-c-c added 27 commits June 23, 2023 11:43
@j-c-c
Update pipeline annotation.
91998b2
@j-c-c
update experiments code comments
e9507f8
@j-c-c
Add default param description to docstring
fb80952
@j-c-c
refactor symmetry_group setter logic.
65380e3
@j-c-c
refactor _reset_orientation
578202c
@j-c-c
Remove misplaced raise in _populate_symmetry_group. Test symmetry_gro…
4e511a9 
…up for RelionSource.
@j-c-c
tox
7f71e11
@j-c-c
remove redundant import
7d3abc5
@j-c-c
symmetry_parser ~~> from_string
a000353
@j-c-c
symmetry_type ~~> to_string
5111796
@j-c-c
use about_axis. CyclicSymmetryGroup ~~> CnSymmetryGroup.
4d7f1a9
@j-c-c
Dihedral ~~> Dn
b1f1eb0
@j-c-c
Tetrahedral ~~> T
dfa69b3
@j-c-c
Octahedral ~~> O
b6a4237
@j-c-c
Symmetry Group ~~> symmetry group
17c9d18
@j-c-c
remove unnecessary transpose.
d286933
@j-c-c
use np.concatenate()
14db9e9
@j-c-c
update error message to indicate symmetry group can be a string.
1b5c092
@j-c-c
replace np.multiply() with *
bd19790
@j-c-c
change symmetry_group setter to a private method.
71b8a0b
@j-c-c
remove _images() method from OrientedSource
d13166d
@j-c-c
remove hardcoded batch_size
2aa65c3
@j-c-c
black
97d9784
@j-c-c
fix test for volume symmetry groups.
5a27094
@j-c-c
tox
b8be0c9
@j-c-c
IdentitySymmetryGroup
0ec76e0
@j-c-c
Comment about chiral symmetry groups.
a16e340
janden
janden approved these changes Jun 26, 2023
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.

LGTM!

@j-c-c j-c-c merged commit e1fe235 into develop Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janden janden janden approved these changes

@garrettwrong garrettwrong garrettwrong 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