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.

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

Fixes for normalization issues #214

Merged
merged 7 commits into from
Jan 17, 2025
Merged

Fixes for normalization issues #214

merged 7 commits into from
Jan 17, 2025

Conversation

AsmaTANABEN
Copy link
Collaborator

@AsmaTANABEN AsmaTANABEN commented Nov 21, 2024

Adjust normalization of density compensation coefficients and adding Cufinufft Pipe function authored by Chaithya G.R.
Resolves #211

…. Add Cufinufft Pipe function authored by Chaithya G.R.
Copy link
Member

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

Ideally we have some tests as well ?

@chaithyagr
Copy link
Member

I would strongly recommend adding the changes for cufinufft in #195 as it is tested there. Also, that will allow this PR to be merged smoothly without waiting for finufft/cufinufft releases.

@paquiteau
Copy link
Member

Questions for you: can this normalization rule be used for the other density compensations methods (e.g. voronoi) ?

@chaithyagr
Copy link
Member

Questions for you: can this normalization rule be used for the other density compensations methods (e.g. voronoi) ?

Answer for you: Yes :). It gives stable results.

Copy link
Member

@chaithyagr chaithyagr left a comment

Choose a reason for hiding this comment

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

LGTM, assuming it matches same as tfkbnufft and tensorflow-nufft. I will rely on you for it @AsmaTANABEN , I dont think we need those tests here run regularly.

@chaithyagr
Copy link
Member

I think we can merge when green and @paquiteau lets make a release?

@chaithyagr
Copy link
Member

I think this can be merged right?

@paquiteau
Copy link
Member

We could merge it yes. However I think this normalization should be refactored to be generalizable to the other density compensation methods (e.g. voronoi, cell-counting etc.). If this is blocking we can post-pone this refactoring to another PR.

@chaithyagr chaithyagr changed the title Fixes for issue #211 Fixes for normalization issues Jan 16, 2025
@chaithyagr chaithyagr merged commit 637b3ad into master Jan 17, 2025
12 checks passed
@chaithyagr chaithyagr deleted the fix_issue_211 branch January 17, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent density-compensation normalization leading to signal instability
3 participants