Skip to content

Conversation

@j-c-c
Copy link
Collaborator

@j-c-c j-c-c commented Jan 11, 2024

This PR adds symmetry boosting to MeanEstimator and WeightedVolumesEstimator.

Boosting is controlled by a flag in the *Estimator's init, which is True (on) by default. Then a symmetry_group is obtained from the provided ImageSource and passed through *Estimator.src_backward() to source.image.im_backward() and finally to image.image.backproject() where boosting of the images and rotations occurs.

Additionally, boosting takes place when computing the Fourier kernel used in mean estimation.

@j-c-c j-c-c added the enhancement New feature or request label Jan 11, 2024
@j-c-c j-c-c self-assigned this Jan 11, 2024
@codecov
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (18fd19c) 88.80% compared to head (3a7e103) 88.82%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1072      +/-   ##
===========================================
+ Coverage    88.80%   88.82%   +0.02%     
===========================================
  Files          126      126              
  Lines        12001    12024      +23     
===========================================
+ Hits         10657    10680      +23     
  Misses        1344     1344              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@j-c-c j-c-c linked an issue Jan 23, 2024 that may be closed by this pull request
@j-c-c j-c-c requested a review from garrettwrong January 23, 2024 19:24
@j-c-c j-c-c force-pushed the boosting branch 2 times, most recently from b258c81 to 8eb0165 Compare January 26, 2024 18:56
@j-c-c
Copy link
Collaborator Author

j-c-c commented Jan 29, 2024

@garrettwrong this is ready for you to take a look.

Following up on dev meeting discussion: Implementing the boosting using a loop as opposed to duplicating images (and weights) came with a significant speed up (on laptop) for larger sets of images and high symmetry order. Here are some timings using image duplication with tiling (t_old) and using the loop (t_loop) for various n and symmetric order:

backproject_timing

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, this will be nice to have. Can revisit 10081. Very minor changes.

i,
self.weights[:, k],
symmetry_group=symmetry_group,
) / (self.src.n * sym_order)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why apply 1/sym_order here instead of in backproject?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No reason other than it made sense to me at the time. This is where we are scaling by number of images and boosting increases the number of images. It also pairs nicely with scaling on line 187.

@j-c-c j-c-c requested a review from garrettwrong February 1, 2024 20:09
garrettwrong
garrettwrong previously approved these changes Feb 5, 2024
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 the updates!

I'll try to re-attempt the 10081 gallery example with this branch soon (and the reduction in images that would be induced by boosting as discussed). Depending on what happens there, and with Joakim's review pass, I might ask you to update this PR or make another one with minor updates for the gallery.

@j-c-c j-c-c marked this pull request as ready for review February 6, 2024 15:02
@j-c-c j-c-c requested a review from janden as a code owner February 6, 2024 15:02
janden
janden previously approved these changes Feb 6, 2024
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 great!

I feel like I've seen this symmetry group parsing code before. May make sense to have a parsing function (SymmetryGroup.from_string) to encode that logic (including the special case of None → IdentitySymmetryGroup).

@j-c-c j-c-c dismissed stale reviews from janden and garrettwrong via 3a7e103 February 8, 2024 13:41
@j-c-c j-c-c mentioned this pull request Feb 8, 2024
@j-c-c
Copy link
Collaborator Author

j-c-c commented Feb 8, 2024

I feel like I've seen this symmetry group parsing code before. May make sense to have a parsing function (SymmetryGroup.from_string) to encode that logic (including the special case of None → IdentitySymmetryGroup).

Sounds good. Added issue #1083. I'll swing back around and clean that up (in addition to some other symmetry_group issues #1081 I noticed during this PR)

Thanks!

@j-c-c j-c-c merged commit c69be08 into develop Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Orientation and Boosting Sources

4 participants