Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Found some differences in the application of fuzzy_mask. I will retest to confirm, but it appears applying these two patches will reproduce the image stack going into the Polar Fourier transform just prior to the CL search and reduce the CL errors wrt MATLAB.

@garrettwrong garrettwrong added bug Something isn't working cleanup labels Sep 3, 2024
@garrettwrong garrettwrong self-assigned this Sep 3, 2024
@garrettwrong
Copy link
Collaborator Author

Confirmed this greatly improved the CL indices wrt MATLAB reference.

@garrettwrong garrettwrong requested a review from j-c-c September 3, 2024 15:48
Copy link
Collaborator

@j-c-c j-c-c 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 to me. Now I'm wondering where I got that default from. Either way, I'm glad this gets us one step closer. Thanks for fixing this!

@garrettwrong
Copy link
Collaborator Author

The default was the same default as their function, so that is fine. It just happened that in use the default was not the value used 🤸‍♂️ .

https://github.com/PrincetonUniversity/aspire/blob/760a43b35453e55ff2d9354339e9ffa109a25371/sinograms/mask_fuzzy.m#L9

@garrettwrong garrettwrong marked this pull request as ready for review September 3, 2024 19:04
@garrettwrong garrettwrong requested a review from janden as a code owner September 3, 2024 19:04
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

@garrettwrong garrettwrong merged commit 8b69fe7 into develop Sep 4, 2024
@garrettwrong garrettwrong deleted the cl_mask_patch branch September 4, 2024 13:53
@garrettwrong garrettwrong mentioned this pull request Sep 6, 2024
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