Rt/ref shim opt deconstruct classes #153
Conversation
…thod into new package +b0shim/+compute/spherical_harmonics.m
…thod to +b0shim/+compute/siemens_basis.m
+b0shim/+compute/siemens_basis.m
Outdated
| % | ||
| % INPUTS | ||
| % | ||
| % orders |
There was a problem hiding this comment.
i would like very much to see the type indicated for each variable. This is super useful for people not familiar with the code. I've suggested it in this PR (https://github.com/shimming-toolbox/shimming-toolbox/pull/145/files#diff-f7c3aae9cf7ffff5e623bbb43d946a50), which we should merge at some point.
There was a problem hiding this comment.
since matlab isn't strongly typed, we need a convention to indicate generic variables (e.g. numeric to encompass double, single, int, etc.?)
it's also a bit unfortunate since, for instance, orders in this case should be integers — but doesn't need to be typed as such, so suggesting the specific type is a bit misleading...
There was a problem hiding this comment.
discussion moved here: shimming-toolbox/shimming-toolbox#145 (comment)
+b0shim/+compute/siemens_basis.m
Outdated
| % distance from iso/origin to adopted reference point [units: mm] | ||
| r = [1 1 1 1 sqrt(2) sqrt(2) 1 sqrt(2)] ; | ||
|
|
||
| % invert polarity of certain terms: |
There was a problem hiding this comment.
any refs here would be welcome
There was a problem hiding this comment.
Hmm... none available, just worked this out myself^
There was a problem hiding this comment.
in that case i would add something like: "(found empirically)". just for transparency.
| % invert polarity of certain terms: | ||
| sh(:,:,:,[2,3,5,8]) = -sh(:,:,:,[2,3,5,8] ) ; | ||
|
|
||
| orders = [1 1 1 2 2 2 2 2] ; |
There was a problem hiding this comment.
maybe mention in the header that currently the function only works for up to 2nd SH order, and open a feature issue to cover 2+ orders?
| % | ||
| % 1. For now, `orders` is, in fact, ignored: fixed as [1:2]—which is suitable | ||
| % for the Prisma (presumably other Siemens systems as well)—however, the | ||
| % 3rd-order shims of the Terra should ultimately be accommodated too. (Requires |
There was a problem hiding this comment.
ah! i'm just seeing this-- scrap this comment then.
i would still open an issue-- much more convenient for project management (than having someone open each .m file and look for "todo" fields)
+b0shim/+compute/siemens_basis.m
Outdated
| % vector of non-negative integers (`[0:1:n]` yields harmonics up to n-th | ||
| % order) | ||
| % | ||
| % X, Y, Z |
There was a problem hiding this comment.
these are separate input params (not a single ndarray), so i would do:
% X (numeric)
% X-coordinate in the patient-coordinate system (i.e. DICOM reference, units of mm)
%
% Y (numeric)
% Y-coordinate in the patient-coordinate system (i.e. DICOM reference, units of mm)
%
% Z (numeric)
% Z-coordinate in the patient-coordinate system (i.e. DICOM reference, units of mm)
There was a problem hiding this comment.
and to suggest that both 2d or 3d work, i would update the usage above:
% basis = siemens_basis( orders, X, Y )
% basis = siemens_basis( orders, X, Y, Z )
There was a problem hiding this comment.
and to suggest that both 2d or 3d work, i would update the usage above:
% basis = siemens_basis( orders, X, Y ) % basis = siemens_basis( orders, X, Y, Z )
"2d" refers to computing the basis on a single slice—still need the Z coordinates in either case
There was a problem hiding this comment.
and to suggest that both 2d or 3d work, i would update the usage above:
% basis = siemens_basis( orders, X, Y ) % basis = siemens_basis( orders, X, Y, Z )"2d" refers to computing the basis on a single slice—still need the Z coordinates in either case
ha! another good example that expliciting what type/size is expected is important for people not familiar with the code (i.e., me 😅).
|
@jcohenadad on this note, can we merge? |
This PR will be part of a series dedicated to refactoring shim-related code in the current Coils subdir.
To break-up the immense refactor and hopefully facilitate code review, this initial PR simply migrates a couple
ShimOptmethods into standalone functions within a new package.To review/discuss:
subpackage folder name
+b0shim/+compute+b0shim/+math+computeso that a function call looks likebasis = b0shim.compute.spherical_harmonics( ... )rather than something likebasis = b0shim.math.compute_spherical_harmonics( ... )documentation format
Note: Code for computing coil induction fields via Biot-Savart can probably be added to this same subpackage (possibly in the same PR...)
(Also, p.s. some of the code is inherited so it doesn't completely follow the style convention)