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

Density compensation #66

Merged
merged 14 commits into from
Dec 15, 2023
Merged

Density compensation #66

merged 14 commits into from
Dec 15, 2023

Conversation

paquiteau
Copy link
Member

This refactors and augments the density compensation routines.

  • ✔️ density is now a proper module, at the same level as operator and trajectories.
  • ✔️ Added new "cell counting" method, which is purely geometric, and runs fast enough in 3D.
  • ⚠️ Pipe's method implementation with cufinufft has been removed (too hacky, and we want to follow the main branch of (cu)finufft).
  • 🚀 There is tests!
  • 👀 Density compensation methods all have the same signature density_method(traj, shape, **kwargs).

Not considered in this PR:

  • Adding more density compensation method. Radio-astronomy have a lot of methods to consider and their naming can be confusing12
  • Registering all the density compensation methods (like we do for the operators, for now we only have 3)

Footnotes

  1. https://casadocs.readthedocs.io/en/stable/notebooks/synthesis_imaging.html#Data-Weighting

  2. https://wsclean.readthedocs.io/en/latest/weighting.html

@philouc
Copy link
Collaborator

philouc commented Dec 15, 2023

Good job @paquiteau

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.

I dont get the last point in your PR description, we technically have the same interface for density estimation?

@@ -139,7 +139,7 @@ jobs:
run: |
cd $RUNNER_WORKSPACE
rm -rf finufft
git clone https://github.com/chaithyagr/finufft --branch chaithyagr/issue306
git clone https://github.com/flatironinstitute/finufft
Copy link
Member

Choose a reason for hiding this comment

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

Is there no pip install?

Copy link
Member Author

Choose a reason for hiding this comment

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

As the development of cufinufft is very active, I prefer to track directly the repo.

Copy link
Member

Choose a reason for hiding this comment

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

We might also need to test against the latest release version as that's what users use.

@paquiteau
Copy link
Member Author

I dont get the last point in your PR description, we technically have the same interface for density estimation?

By registering, I meant to have something to collect all density compensation methods and then do a

get_density("cell_count")(traj, ...)  

@paquiteau paquiteau requested a review from chaithyagr December 15, 2023 11:39
@paquiteau paquiteau force-pushed the density-comp branch 4 times, most recently from 365c4ab to 95fc0ef Compare December 15, 2023 12:43
@paquiteau paquiteau merged commit 9e2e996 into master Dec 15, 2023
chaithyagr pushed a commit to chaithyagr/mri-nufft that referenced this pull request Apr 11, 2024
* feat: refactor density compensation to a module.

* feat: add decorator to flatten trajectories.

* feat: add ultra fast cell counting method.

* refactor: use the flat_traj decorator.

* refactor: make pipe a classmethod.

* fix: special case for 2d trajectories.

* feat: enforce common api for density compensation functions.

* feat: move density module up.

* feat(density): add test.

* feat(cufinufft)!: drop pipe implementation and follow upstream.

* fix: update module level for density.

* fix: cleanup

* feat(ci): Install most recent cufinufft

* fix(ci): copy libcufinufft.so at the right place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants