Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Closes #1138 , see issue for more detailed list of work.

@garrettwrong garrettwrong added bug Something isn't working enhancement New feature or request CI Continuous Integration cleanup Optimization Performance or Resource Optimzation GPU labels Jun 24, 2024
@garrettwrong
Copy link
Collaborator Author

garrettwrong commented Jun 24, 2024

Noting this is for the next release, tentatively v0.13.0.

Copy link
Collaborator Author

@garrettwrong garrettwrong left a comment

Choose a reason for hiding this comment

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

self review punch list.

@garrettwrong garrettwrong force-pushed the hack24 branch 2 times, most recently from 81761b5 to aa135c9 Compare July 1, 2024 20:20
@garrettwrong garrettwrong requested a review from j-c-c July 3, 2024 14:30
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.

This looks great. Just a couple things.

_z = nufft(im, self.grid_xy, epsilon=self.epsilon) * self.h**2
_z = _z.reshape(num_img, self.num_radial_nodes, self.num_angular_nodes // 2)
z[:, :, : self.num_angular_nodes // 2] = _z
z[:, :, self.num_angular_nodes // 2 :] = np.conj(_z)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want _z.conj() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that should be better, thanks for catching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually i found a few things right in that area I must have missed. sorry about about. You'll see them in the re-review.

@garrettwrong
Copy link
Collaborator Author

A few changes and rebased with latest (resolves that CI patch conflict I had in here). LMK if you see anything else, thanks!

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. Just one other suggestion that you can take or leave.

j-c-c
j-c-c previously approved these changes Jul 15, 2024
@garrettwrong garrettwrong marked this pull request as ready for review July 15, 2024 19:43
@garrettwrong garrettwrong requested a review from janden as a code owner July 15, 2024 19:43
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.

Looks great! Just a few things.

@garrettwrong garrettwrong enabled auto-merge (rebase) July 23, 2024 16:34
@garrettwrong garrettwrong merged commit b9f263b into develop Jul 23, 2024
@garrettwrong garrettwrong deleted the hack24 branch July 23, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working CI Continuous Integration cleanup enhancement New feature or request GPU Optimization Performance or Resource Optimzation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants