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

Signal recovery optimizer #429

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft

Signal recovery optimizer #429

wants to merge 34 commits into from

Conversation

yixinma9
Copy link
Collaborator

Checklist

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've added relevant tests for my contribution
  • I've updated the documentation and/or added the correct docstrings
  • I've assigned a reviewer

Description

Add signal recovery optimizer

Linked issues

@yixinma9 yixinma9 added the Epic General collection of features that will be sub-divided label Jan 13, 2023
@yixinma9 yixinma9 self-assigned this Jan 13, 2023
@yixinma9 yixinma9 requested a review from po09i January 13, 2023 17:40
Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

There is a lot of comments, but most of them are minor simple tweaks. I have not tested if I can run it, but it looks good overall :)

shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/optimizer/lsq_optimizer.py Outdated Show resolved Hide resolved
shimmingtoolbox/optimizer/lsq_optimizer.py Outdated Show resolved Hide resolved
shimmingtoolbox/optimizer/lsq_optimizer.py Outdated Show resolved Hide resolved
shimmingtoolbox/shim/sequencer.py Outdated Show resolved Hide resolved
shimmingtoolbox/shim/sequencer.py Outdated Show resolved Hide resolved
Copy link
Member

@po09i po09i left a comment

Choose a reason for hiding this comment

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

We should find a way to calculate and pass the Gx, Gy, Gz options so that it does not affect the other "criteria". In the current form, the "mse" would unnecessarily calculate the gradients, pass it to the _scipy_minimize() function and crash when trying to call "mse"'s _criteria_func().

Similarly, in the sequencer, there should be a function that handles all the necessary calculation and necessary plots for this new method in a if that gets executed only if it's this optimizer.

The code is looking good :)

shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
Comment on lines 293 to 294
end_time = time.time()
print("Time taken for shim_sequencer to run is : ", end_time - start_time, "seconds")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
end_time = time.time()
print("Time taken for shim_sequencer to run is : ", end_time - start_time, "seconds")

shimmingtoolbox/cli/b0shim.py Outdated Show resolved Hide resolved
shimmingtoolbox/masking/mask_utils.py Outdated Show resolved Hide resolved
shimmingtoolbox/optimizer/lsq_optimizer.py Outdated Show resolved Hide resolved
shimmingtoolbox/optimizer/lsq_optimizer.py Outdated Show resolved Hide resolved
Comment on lines 290 to 295
coil_Gx_mat = np.reshape(merged_coils_Gx,
(n_channels, -1)).T[mask_erode_vec != 0, :] # masked points x N
coil_Gy_mat = np.reshape(merged_coils_Gy,
(n_channels, -1)).T[mask_erode_vec != 0, :] # masked points x N
coil_Gz_mat = np.reshape(merged_coils_Gz,
(n_channels, -1)).T[mask_erode_vec != 0, :] # masked points x N
Copy link
Member

Choose a reason for hiding this comment

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

What if the acquisition is oblique? The "z gradient" would be in the slice encoding direction, does that have an impact on your calculation? There is some code in the toolbox (shimmingtoolbox.shim.realtime_shim:realtime_shim()) to calculate the gradient in the "true z" direction.

@@ -186,10 +204,12 @@ def _eval_static_shim(opt: Optimizer, nii_fieldmap_orig, nii_mask, coef, slices,
shimmed = np.zeros(unshimmed.shape + (len(slices),))
corrections = np.zeros(unshimmed.shape + (len(slices),))
masks_fmap = np.zeros(unshimmed.shape + (len(slices),))
print('The variable slices is as following: ' + str(slices)) # Currently the code works for --slice sequential shimming, but need to adapt it so that it also works for the --slices volume
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
print('The variable slices is as following: ' + str(slices)) # Currently the code works for --slice sequential shimming, but need to adapt it so that it also works for the --slices volume

4rnaudB and others added 15 commits December 19, 2023 17:17
Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
Co-authored-by: Alex Dastous <47249340+po09i@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic General collection of features that will be sub-divided
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants