Fix: Dim 1 not calculating properly

[heidthecamp]

Fixed an issue that was noticed with sum and product, where an argument of dim=1 would break and show the wrong answer repeating the first calculations. It, however, affected several other opcodes.

List all affected opcodes:

• MAXLOC
• MINLOC
• MAXVAL
• MINVAL
• MEAN
• PRODUCT
• SUM
• ACCUMULATE

When doing sum or product in tdi with multidimensional arrays using the argument dim = 1 would result in an error, doubling the inner array twice.

simply:

TDI> sum(
    [[[1], [2]],
    [[3],[4]]]
,1)

[[3], [3]]
summing 1 and 2 twice, when it should produce the sum of 1 and 2 then 3 and 4.
[[3], [7]]
this would even happen with

TDI> sum([[[1],[2]],[[3],[4]],[[5],[6]]],1)
[[3], [3], [3]]
# Post fix:
[[3], [7], [11]]

A more robust example:

TDI> sum(
[
  [
    [1,2,3,4],
    [5,6,7,8],
    [9,10,11,12]
  ],
  [
    [13,14,15,16],
    [17,18,19,20],
    [21,22,23,24]
  ]
],1)

[[15,18,21,24], [15,18,21,24]]
After applying the fix it is no longer doubling first inner array.
[[15,18,21,24], [51,54,57,60]]

This was easiest to reproduce with a dim of one but if you could contrive of an example where a higher dimension would need to be duplicated you would be able to get those values to repeat as well.

TDI> sum([[[[1],[2]],[[2],[3]]],[[[3],[4]],[[4],[5]]],[[[5],[6]],[[6],[7]]]],2)

[[[3], [5]], [[3], [5]], [[3], [5]]]
[[[3], [5]], [[7], [9]], [[11], [13]]]

The issue stems from the outer most for loop having a ';' in the wrong location causing the set up for several values to be reset every time the loop was evaluated.

[mwinkel-dev]

Hi @heidthecamp -- This change looks good to me. (The outer loop now has the same general structure as the inner loops.).

It is unlikely that customers have decades of archived trees that now rely on the original wrong behavior. Thus, the risk of this fix breaking existing archives is likely very low.

Nonetheless, a major benefit of MDSplus is that trees written decades ago can still be read today. Which means that TDI changes must be scrutinized. So, I will ponder the matter on Monday before submitting an official review / approval.

[mwinkel-dev]

Hi @heidthecamp -- This fix is definitely correct. However, because it affects 8 TDI functions, I am doing some additional spot-checks of the 8 functions using some trees from the C-Mod archive. (It is highly unlikely that anything will go awry with this fix, but it is prudent to confirm all is well.)

[mwinkel-dev]

Hi @heidthecamp -- The loops in the TdiMaxVal.c file have variables associated with "before", "after" and "dim". Those concepts are explained in Ken Klare's comment in the TdiTrans.c file. Unfortunately, the table in Ken's comment was mangled when the source code was reformatted a few years ago. Below is the link to the current mangled table, plus the original table (found in the earliest history). It might be good to fix the formatting of Ken's table as part of this PR.

mangled table

The original table (contains tabs, which should probably be replaced with spaces).

For logical array V[3,4,5], steps in unit sizes:
loop	count @step	no DIM	DIM=0	optim	DIM=1	DIM=2
outer	aft		1 @xx	20 @3	1 @xx	5 @12	1 @60	DIM dimension and number of results
middle	bef		1 @1	1 @3	20 @3	3 @1	12 @1	product of dims after DIM
inner	dim		60 @1	3 @1	3 @1	4 @3	5 @12	product of dims before DIM
Example, ALL(V,0) chooses elements: [0,1,2] [3,4,5] ...
Example, ALL(V,1) chooses elements: [0,3,6,9] [1,4,7,10] [2,5,8,11] / [12,15,18,21] ...
Example, ALL(V,2) chooses elements: [0,12,24,36,48] [1,13...], ... [11,...]

Unfortunately, GitHub doesn't preserve the column alignment in the above table. Looking at the "outer" row and expressing it as a Python dictionary:

{
   "loop": "outer", 
   "count @step": "aft", 
   "no DIM": "1 @xx", 
   "DIM=0": "20 @3", 
   "optim": "1 @xx", 
   "DIM=1": "5 @12", 
   "DIM=2": "1 @60", 
   "Note": "DIM dimension and number of results"
}

[mwinkel-dev]

Hi @heidthecamp -- As you mentioned during today's software team meeting, this problem is not limited to two-dimensional arrays with the specified DIM=1.

If a multi-dimensional array contains N dimensions, then the user can choose to slice the array along any of those dimensions. Meaning that the DIM selected could be any value from 0 through N-1.

[mwinkel-dev]

Tested the fix by creating these three arrays.

TDI> _a1d = ramp(24,1)+1
[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24]

TDI> _a2d = ramp([12,2],1) + 1
[[1,2,3,4,5,6,7,8,9,10,11,12], [13,14,15,16,17,18,19,20,21,22,23,24]]

TDI> _a3d = ramp([4,3,2],1) + 1
[[[1,2,3,4], [5,6,7,8], [9,10,11,12]], [[13,14,15,16], [17,18,19,20], [21,22,23,24]]]

Then applied the 8 functions:

• minloc, maxloc
• minval, maxval
• sum, accumulate, mean
• product

Output for minval and maxval follows:

TDI> minval(_a1d)
1
TDI> maxval(_a1d)
24
TDI> minval(_a1d,0)
1
TDI> maxval(_a1d,0)
24

TDI> minval(_a2d)
1
TDI> maxval(_a2d)
24
TDI> minval(_a2d,0)
[1,13]
TDI> maxval(_a2d,0)
[12,24]
TDI> minval(_a2d,1)
[1,2,3,4,5,6,7,8,9,10,11,12]
TDI> maxval(_a2d,1)
[13,14,15,16,17,18,19,20,21,22,23,24]

TDI> minval(_a3d)
1
TDI> maxval(_a3d)
24
TDI> minval(_a3d,0)
[[1,5,9], [13,17,21]]
TDI> maxval(_a3d,0)
[[4,8,12], [16,20,24]]
TDI> minval(_a3d,1)
[[1,2,3,4], [13,14,15,16]]
TDI> maxval(_a3d,1)
[[9,10,11,12], [21,22,23,24]]
TDI> minval(_a3d,2)
[[1,2,3,4], [5,6,7,8], [9,10,11,12]]
TDI> maxval(_a3d,2)
[[13,14,15,16], [17,18,19,20], [21,22,23,24]]

Output for sum follows:

TDI> sum(_a1d)
300
TDI> sum(_a1d,0)
300

TDI> sum(_a2d)
300
TDI> sum(_a2d,0)
[78,222]
TDI> sum(_a2d,1)
[14,16,18,20,22,24,26,28,30,32,34,36]

TDI> sum(_a3d)
300
TDI> sum(_a3d,0)
[[10,26,42], [58,74,90]]
TDI> sum(_a3d,1)
[[15,18,21,24], [51,54,57,60]]
TDI> sum(_a3d,2)
[[14,16,18,20], [22,24,26,28], [30,32,34,36]]

The other functions also worked correctly.

A minor surprise is that accumulate with the dimension omitted, does treat the input array as a one-dimensional vector however returns the result in the multi-dimensional shape of the input array. This behavior is probably correct.

TDI> accumulate(_a1d)
[1,3,6,10,15,21,28,36,45,55,66,78,91,105,120,136,153,171,190,210,231,253,276,300]
TDI> accumulate(_a2d)
[[1,3,6,10,15,21,28,36,45,55,66,78], [91,105,120,136,153,171,190,210,231,253,276,300]]
TDI> accumulate(_a3d)
[[[1,3,6,10], [15,21,28,36], [45,55,66,78]], [[91,105,120,136], [153,171,190,210], [231,253,276,300]]]

TDI> accumulate(_a2d,0)
[[1,3,6,10,15,21,28,36,45,55,66,78], [13,27,42,58,75,93,112,132,153,175,198,222]]
TDI> accumulate(_a3d,0)
[[[1,3,6,10], [5,11,18,26], [9,19,30,42]], [[13,27,42,58], [17,35,54,74], [21,43,66,90]]]

[mwinkel-dev]

To evaluate the potential impact of this change on historical trees, scanned the entire tree for C-Mod shot 1160930043 for node expressions that used any of the 8 functions, and then evaluated some of those nodes using two versions of MDSplus: unfixed and fixed. The results were the same (because the C-Mod expressions didn't specify a dimension argument).

Of course, there was one exception to the rule. Namely, the following node did specify a dimension. However, the referenced nodes didn't contain any data, so it is a moot issue for this shot. Nonetheless, there is a chance that it might affect other trees, such as the HXR data generated at DIII-D.

Node: LH.HXR.RESULTS:COUNTRATE:CH01
Expr: SUM(.LH.HXR.RESULTS.ENERGY:CH01, 1) * .LH.HXR.PARAMS:GOOD[0]

This is the list of nodes spot-checked for each function:

accumulate() = not found in the tree

minloc() = not found in the tree

minval()
- SPECTROSCOPY.BOLOMETER.RESULTS.FOIL:DRIVE
- SPECTROSCOPY.H0.RETICONS:H0_AURORA14:INPUT_1:ENDIDX

maxval()
- SPECTROSCOPY.BOLOMETER.RESULTS.FOIL:DRIVE
- SPECTROSCOPY.H0.RETICONS:PULSE:PULSE_TIME

mean()
- EDGE.SURFSS.OVERVIEW:M_PVERTICAL

product() = not found in the tree

sum()
- EDGE.GAS.NINJA:PLEN_BOT
- MHD.MAGNETICS.SHOELACE:ANT_I:DC_REMOVED

[mwinkel-dev]

This PR has also been tested by @heidthecamp and @WhoBrokeTheBuild. They too evaluated the potential impact of the PR on historical trees. They said that they did not find any problems.

Their results, plus the testing described in the previous two posts, indicates that this PR is very low risk to historical trees. Meaning that it is unlikely that any researchers at C-Mod were relying on the incorrect results of the 8 functions. It is thus probable that other research institutions also did not use the 8 functions with a dimension argument, thus did not rely on the incorrect results.

Because the new fix works well, and has no obvious impact on the historical C-Mod trees, this PR is now approved.