September 2025 new azimuthal average changes - #549
Conversation
bHimes
left a comment
bHimes
left a comment
There was a problem hiding this comment.
I assume these methods do what they should. My comments are mostly to clean up some easy performance issues with memory allocations (calls to new) and narrowing the scope of local variables which is safer and allows the compiler to better optimize register usage.
|
|
||
| void Image::AverageRotationally( ) { | ||
| // image must be in real space | ||
| bool switchBackToComplex = false; |
There was a problem hiding this comment.
This is a minor one, but even though cisTEM was built assuming only real-valued inputs, we use real-space complex valued images now in some cases, so let's not introduce a variable named like this. Perhaps input_image_in_fourier_space. The logic would all stay the same.
| long number_of_rings = this->logical_x_dimension; | ||
| //float edge_value = current_image->ReturnAverageOfRealValues(std::min(current_image->physical_address_of_box_center_x - 2, current_image->physical_address_of_box_center_y - 2), true); | ||
| float edge_value; | ||
| double* ring_axis = new double[number_of_rings]; |
There was a problem hiding this comment.
Two problems here:
1 - Calling "new" is very slow.
2 - We should prefer to not add more raw pointers to the project if possible. It's okay to ignore this here i guess.
// Only make one system call
double* ring_buffer = new double[number_of_rings * 3];
// Zero the array (currently done in a later loope)
ZeroArray(ring_buffer, int(number_of_rings * 3))
// Get (non-owning) pointers to slices in that memory buffer. Only the buffer needs to be deallocated.
double* ring_axis = &ring_buffer[number_of_rings*0];
double* ring_values = &ring_buffer[number_of_rings*1];
double* ring_weight = &ring_buffer[number_of_rings*2]
//
// existing code is unchanged
//
// cleanup
// delete[] ring_buffer;| for ( counter = 0; counter < number_of_rings; counter++ ) { | ||
| ring_axis[counter] = 0.0 + counter * (this->ReturnMaximumDiagonalRadius( ) - 0.0) / float(number_of_rings - 1); // Diagonal because we're using the physical address (0 is the upper left corner) | ||
| // = counter * (sqrt(pow(physical_address_of_box_center_x, 2) + pow(physical_address_of_box_center_y, 2) + pow(physical_address_of_box_center_z, 2))); | ||
| ring_values[counter] = 0; // What is this? |
There was a problem hiding this comment.
I think the answer to "what is this?" is probably that the buffer arrays are being forced to zero in this loop. This is added above following initialization so can be removed here for these two lines.
| } | ||
|
|
||
| // edge radius in real space is 0.5*logical_dimension | ||
| long edge_bin = long((0.5 * this->logical_x_dimension - ring_axis[0]) / (ring_axis[1] - ring_axis[0])); |
There was a problem hiding this comment.
In this method, it is clear we only operate on self, so using "this->" is probably overkill. Up to you, but I would remove them here as it is a bit of clutter.
|
|
||
| // z-dim first; that's each image in the volume (looking "through" the image rather than at the side) | ||
| // Nested loops mean move across the image starting at the first image (z), the first row of pixels, going column by column (left to right across the image, going from top to bottom) | ||
| for ( z = 0; z < this->logical_z_dimension; z++ ) { |
There was a problem hiding this comment.
Are the loop values used outside the loop? If not, prefer for (int z = 0 ... ) to restrict "z" scope to the loop it is used in. I realize there is lots of code in the project that intializes everything up top, but that is outdated practice and shouldn't be built on. It is actually LESS performant, which is the original reason for declaring everything up top.
| counter = 0; | ||
|
|
||
| for ( z = 0; z < this->logical_z_dimension; z++ ) { | ||
| for ( y = 0; y < this->logical_y_dimension; y++ ) { |
There was a problem hiding this comment.
Contrary to what I mentioned above about not initializing a variable before we need to (loop counters) In this case, powf is a somewhat expensive operation, and is the same for every y value in the loop. Also, the "f" means it is for a float, but you are casting the input to a double (which must either be ignored or cast back to a float.) If you want it to be double, you should use "pow". Same for sqrt/sqrtf
for ( z = 0; z < this->logical_z_dimension; z++ ) {
float z_radius_squared = powf(float(central_z_pixel - z), 2));
// or
double z_radius_squared = pow(double(central_z_pixel - z), 2));
for ( y = 0; y < this->logical_y_dimension; y++ ) {
float/double y_radius_squared = ...; // same
for ( x = 0; x < this->logical_x_dimension; x++ ) {
radius = sqrtf(powf(double(central_x_pixel - x), 2) + y_radius_squared + z_radius_squared);
|
|
||
| if ( is_in_real_space ) { | ||
| this->ForwardFFT( ); | ||
| do_reverse_FFT = true; |
There was a problem hiding this comment.
see note on the next method, this is a better name than "switchBackToComplex" however it should be do_inverse_FFT if you prefer to match nomenclature with the rest of the world or do_backward_FFT if you prefer to match existing cisTEM nomenclature.
| } | ||
|
|
||
| void Image::ApplyRampFilter( ) { | ||
| float x; |
There was a problem hiding this comment.
Some notes on this in the other method. Prefer to declare this in the narrowest scope possible (float z just inside the z-loop, float y just inside the y-loop, etc.) This is safer and more performant.
0e72765 to
b4ae8a4
Compare
|
Hi @Heidy-Elkhaligy - I saw a force pushed commit and several conversations marked resolved that were not resolved. I hope this is just an issue with using git/github (which can be a pain in the butt at first.) If you have any questions about the code review process or the mechanics of how to respond to/push up changes to code review requests, please let me know. I'm happy to also set aside 10-15 min for a quick Zoom to discuss. If on the other hand, you did mean to mark my comments as resolved without addressing the code or addressing the comments either, please let me know. If you aren't planning on future contributions and just want to "get this done" I'm happy to just accept your PR and then fix the code myself, which is much easier than going through code review. I only made comments on changes to the Image class as this might affect the project more broadly. If you would like a review of src/programs/azimuthal_average/azimuthal_average.cpp I can do that as well, but am perfectly happy to let that exist in whatever state you find acceptable as it only affects itself. |
Sorry @bHimes, I am working with @Heidy-Elkhaligy on this PR in-office -- we have been going through the comments you've suggested and implementing them, @Heidy-Elkhaligy is also just going through some of her azimuthal_average.cpp code as well to check if there's anything else she wants to correct there before pushing. In the future I will be sure to drop a comment sooner to let you know. But, apologies for not letting you know sooner, and the changes will probably be pushed sometime this week! |
|
@bHimes Thanks for pointing this out! I thought that resolving comments locally wouldn’t mark them as resolved until after I push the changes, that was my mistake 😊. It’s my first time working on a pull request, so Tim @twagner9 was helping me on Friday. |
|
No worries! working with git/gh has a pretty steep learning curve. Someday
I hope to understand it : )
…-Ben
------------------------
Benjamin A. Himes, PhD
Managing Member
Stochastic Analytics, LLC
------------------------
On Mon, Sep 15, 2025 at 10:47 AM Heidy-Elkhaligy ***@***.***> wrote:
*Heidy-Elkhaligy* left a comment (timothygrant80/cisTEM#549)
<#549 (comment)>
@bHimes <https://github.com/bHimes> Thanks for pointing this out! I had
thought that resolving comments locally wouldn’t mark them as resolved
until after I pushed the changes, that was my mistake 😊. It is my first
time working on a pull request, so Tim @twagner9
<https://github.com/twagner9> was helping me on Friday.
I will push the current changes so you can see them now. I really
appreciate your patience and the offer to help walk me through the process.
—
Reply to this email directly, view it on GitHub
<#549 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXPLGDYNAKW5UZKL2ZIFZL3S3GOPAVCNFSM6AAAAACGIFSUIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEOJSGU3TANBQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
image.cpp added AverageRotationally and ApplyRampFilter( ) functions azimuthal_average.cpp now contains RASTR and SPOT-RASTR options
1. Renamed variables for clarity: - switchBackToComplex → input_image_in_fourier_space - do_reverse_FFT → do_backward_FFT 2. Replaced raw pointers with smart pointers and removed delete[] calls 3. Restricted loop variables to local scope 4. Optimized powf/sqrtf usage by removing redundant powf calls
1. Renamed variables for clarity 2. Replaced some raw pointers with smart pointers and removed delete[] calls 3. Restricted loop variables to local scope
03676c6 to
8bbaf63
Compare
|
@bHimes, I implemented some of the changes you suggested in image.cpp within azimuthal_average.cpp. Sorry for the force push; it was necessary because we had to rebase. I’d appreciate your help reviewing those updates. |
|
@bHimes I'm also going on a month-long vacation starting this week. Therefore, there's no rush to review the code, as I won't be able to make any changes during that time. Thank you again for your help and support. |
|
Awesome! My kinda traveler. Happy to wait if you'd like to learn more about the process. If you like, i'm also happy to pull in your changes and make any adjustments for you |
|
Awesome, thank you! I’d love to learn more about the process and the logic behind the changes so I can avoid making the same mistakes in the future. I’m not a programmer or computer scientist, and my coding skills are pretty basic, so understanding the reasoning would really help me improve. If you decide to go ahead and finish the pull request, I’d really appreciate it if you could point out what I missed or did incorrectly, that would be super helpful for me to learn from! |
|
@bHimes - are you happy with the state of this now? |
|
i'll look it over again tomorrow |
|
Included in PR #572, only modified RampFilter method in the Image class, otherwise, the other changes @Heidy-Elkhaligy made looked fine. |
4 participants
image.cpp added AverageRotationally and ApplyRampFilter( ) functions azimuthal_average.cpp now contains diameTR, RASTR and SPOT-RASTR options
Description
Old version: Alignment was performed via iterative real-space cross-correlation with angle scanning until convergence.
New version: Alignment now supports an initial alignment step using autocorrelation and Fourier Transform (FT), which generates a reference image for a second round of alignment using cross-correlation, providing more robust and faster convergence
New additions in the new version:
Fixes # (issue)
I have rebased my feature branch to be current with the master branch using to minimize conflicts and headaches
Which compilers were tested
These changes are isolated to the
How has the functionality been tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
Checklist: