Adding gradient of a vector to the Fourier collections class - #123
Conversation
Cyclomatic Complexity CheckL 97 (C 22-41): The McCabe cyclomatic complexity of 'initialiseSensorData' is 19. L 59 (C 14-17): The McCabe cyclomatic complexity of 'curl' is 2. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #123 +/- ##
===========================================
- Coverage 100.00% 98.71% -1.29%
===========================================
Files 59 48 -11
Lines 911 1093 +182
===========================================
+ Hits 911 1079 +168
- Misses 0 14 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I realise the case with a N-dimensional kappa is not being tested for this method. However, I cannot see where it is tested for the gradient or divergence methods either. If it is tested, I could follow a similar approach for using the N-dimensional k-space correction so please point me to the right place in the code for this. |
| % df = [dfxdx dfxdy dfxdz; | ||
| % dfydx dfydy dfydz; | ||
| % dfzdx dfzdy dfzdz] |
There was a problem hiding this comment.
If the operator is defined as the outer product between [dx; dy; dz] * [fx, fy, fz] the output would be the transpose of this. Is there a code reason for having it this way around, for example, when you combine with other tensors?
There was a problem hiding this comment.
Not really. I normally use this notation and when looking through the literature it also seemed like a correct approach see this for instance. The numerator layout (as above) is commonly used for Jacobians, which I guess is what I had in the back of my mind.
Happy to use whatever convention you prefer and I will make sure the convention is consistent in the solver as well.
| % padded grid. The output can be returned on a spatially staggered grid by | ||
| % setting the optional |Staggering| argument. |
There was a problem hiding this comment.
Depending on what we do with the staggering flags (see later comment), we should make some comments about the staggering scheme that is used. Currently, it's not generic, but connected to the scheme we are using in the elastic code (similarly for the divergenceSplitTensor function).
There was a problem hiding this comment.
Agree, lets see what we decide regarding the staggering in general and how we want to generalise the API, then we will address this as appropriate.
| % You should have received a copy of the GNU Lesser General Public License | ||
| % along with k-Wave-II. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| function df = gradientVector(obj, f, options) |
There was a problem hiding this comment.
Aside from staggering, this could be implemented by calling the gradient function on each of the vector components. However, this would mean we would need to have a more general way of defining the staggering in the gradient function.
If we could think of a nice API for this, it would potentially allow us to make the functions more general, including, the staggering used in divergenceSplitTensor (which currently has the elastic code staggering built in). Noting that in 3D, if you allow forward, backward, and none, there are 27 combinations, so just a look up table would be cumbersome.
The nice thing about re-using gradient is that any low level optimisations we do on this function would then be automatically used by gradientVector.
There was a problem hiding this comment.
Maybe something like 'staggerXPos' and 'staggerXNeg' as options (yes, there would be a lot of options this way, but it would be rather transparent?) could work, where XPos means YNeg and ZNeg and XNeg would mean YPos and ZPos? I lookup table is more elegant programmatically, but less readable / transparent to the user so I'm not sure what the 'best' option is.
I understand the issue with the staggering here though being elastic-solver specific, but given that calling a gradient of a vector would return a tensor, which I "assume" would be used for a stress component also in other solves, would it not be the case that this staggering strategy, which mathematically not being general, would always be used in practice by other solvers using this operator?
Might there also be some advantages to having the gradient and gradientVector operators separated, such as having the ability to apply different low-level optimisation strategies directly on the 5D array rather than the components of it? From the API perspective, it could also help end users knowing that from a gradient operator you always get a vector, and from a gradientVector operator you always get a tensor (I personally do not like that and would prefer to have a single operator with the output dependent on the input, but it might be worth thinking what would be better for end users as well).
I will let @btreeby and @bencox have a think and whatever the final decision is will implement it in the next development phase.
There was a problem hiding this comment.
As there seems to be some discussion which will have to take place, should I mark this as draft in the meanwhile?
Co-authored-by: Bradley Treeby <b.treeby@ucl.ac.uk>
Added a method to the FourierCollections class to calculate the gradient of a vector field, returning a matrix. This is a general gradient operator on a vector field (see issue #122).
The unit tests added follow the same structure and functions used for the gradient and divergence unit tests.
In the current code base, this method does not have any use. It will form a component of subsequent solvers such as the ElasticSolver (see issue #120) and potentially more in the future (such as a Maxwell solver).