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

Dispatch density methods in operators #68

Merged
merged 16 commits into from
Jan 10, 2024
Merged

Dispatch density methods in operators #68

merged 16 commits into from
Jan 10, 2024

Conversation

paquiteau
Copy link
Member

This PR allow to dispatch the choice for density compensation estimation. It also provides a get_density methods and can show which density compensation methods are available.

Also, It provides a generic purpose MethodRegister that could be used for registering other collection of methods (trajectories functions for example).

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.

Great work! Needs an example surely though to better understand the usage

@@ -35,7 +35,7 @@
display_3D_trajectory,
)

from .density import voronoi, cell_count, pipe
from .density import voronoi, cell_count, pipe, get_density
Copy link
Member

Choose a reason for hiding this comment

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

In documentation I recommend mentioning a bit about each including computation times

return new_traj


class MethodRegister:
Copy link
Member

Choose a reason for hiding this comment

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

Was this abstraction really needed and helpful? Especially when we have only 3 density methods and also in which only 2 are used. Although, i understand you are trying to be future proof.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although, i understand you are trying to be future proof.
Yep, Maybe we would like to collect other type of methods as well at one point (e.g. trajectories)

if not callable(method):
raise ValueError(f"Unknown density method: {method}")

self.density = method(self.samples, self.shape, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Under the current framework, how can we change shape? In case I want to compute density for a larger OSF i.e. COnventionally Pipe is done for OSF=2 (as all NUFFT is done for OSF=2)

Copy link
Member

Choose a reason for hiding this comment

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

Also i am curious how can we pass additional arguments through these wrappers? I see **kwargs, but how can we use them while say instancing a NUFFT operator? Is this doable?

@paquiteau paquiteau force-pushed the density_register branch 4 times, most recently from 6ebc269 to 677055a Compare January 8, 2024 15:43
@chaithyagr
Copy link
Member

The call for pipe method must be fixed to make sure we are good with gpunufft 2,0

grid_op.op(grid_op.adj_op(density_comp, True), True)
-->
grid_op.op(grid_op.adj_op(density_comp, None, True), None, True)

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.

Seems good to me, Great work! Maybe make a release so that I can tag in pysap-mri?

@paquiteau paquiteau merged commit 2f39bcf into master Jan 10, 2024
8 checks passed
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.

2 participants