R-js/blasjs

Question about slice function

Schultzer opened this issue · 4 comments

Hi,

First of all thanks for all this work. I have a question about

im[base + (i - rowStart)] = this.i[coorJ + i];

is there a reason why you would use undefined behavior here?

In the first round base + (i - rowStart) == -1 which result in a undefined behavior and then skips by one and turns the im = 0.

if this is the purpose wouldn't it be better to wrap it around an if statement to make it more clear what the desired behavior would be?

Hello,

thank you for your interest,

first, its very clear, the code is wrapped in a small for loop, look what the starting (and ending) value of i is. just 4 lines above. (421)

for (let i = rowStart; i <= rowEnd; i++) {`     // <-- where "i" comes from
   // console.log(j, coorJ + i, base + i, r[coorJ + i]);
   re[base + i] = r[coorJ + i];
    if (this.i && im) {
         im[base + (i - rowStart)] = this.i[coorJ + i];   <-- the line of your prev question
    }
}

this makes base + (i - rowStart) == 0

The mapping is neccesary because, Not all langauges have zero based indexed arrays/matrices (fortran it is flexible and defaults to 1).

(rowBase and colbase properties)
https://github.com/R-js/blasjs#matrix

If base + (i - rowStart) == 0 is assume to always return 0 on the first round, then I think we have a bug because when I added this line to the loop console.log(`re: base + i = ${base + i} im: base + (i - rowStart) = ${base + (i - rowStart)}`);

it clearly shows that on the first round base + (i - rowStart) = -1

fixtures.matrix_mxn(6, 6).slice(1, 6, 1, 6)
re: base + i = 0 im: base + (i - rowStart) = -1
re: base + i = 1 im: base + (i - rowStart) = 0
re: base + i = 2 im: base + (i - rowStart) = 1
re: base + i = 3 im: base + (i - rowStart) = 2
re: base + i = 4 im: base + (i - rowStart) = 3
re: base + i = 5 im: base + (i - rowStart) = 4
re: base + i = 6 im: base + (i - rowStart) = 5
re: base + i = 7 im: base + (i - rowStart) = 6
re: base + i = 8 im: base + (i - rowStart) = 7
re: base + i = 9 im: base + (i - rowStart) = 8
re: base + i = 10 im: base + (i - rowStart) = 9
re: base + i = 11 im: base + (i - rowStart) = 10
re: base + i = 12 im: base + (i - rowStart) = 11
re: base + i = 13 im: base + (i - rowStart) = 12
re: base + i = 14 im: base + (i - rowStart) = 13
re: base + i = 15 im: base + (i - rowStart) = 14
re: base + i = 16 im: base + (i - rowStart) = 15
re: base + i = 17 im: base + (i - rowStart) = 16
re: base + i = 18 im: base + (i - rowStart) = 17
re: base + i = 19 im: base + (i - rowStart) = 18
re: base + i = 20 im: base + (i - rowStart) = 19
re: base + i = 21 im: base + (i - rowStart) = 20
re: base + i = 22 im: base + (i - rowStart) = 21
re: base + i = 23 im: base + (i - rowStart) = 22
re: base + i = 24 im: base + (i - rowStart) = 23
re: base + i = 25 im: base + (i - rowStart) = 24
re: base + i = 26 im: base + (i - rowStart) = 25
re: base + i = 27 im: base + (i - rowStart) = 26
re: base + i = 28 im: base + (i - rowStart) = 27
re: base + i = 29 im: base + (i - rowStart) = 28
re: base + i = 30 im: base + (i - rowStart) = 29
re: base + i = 31 im: base + (i - rowStart) = 30
re: base + i = 32 im: base + (i - rowStart) = 31
re: base + i = 33 im: base + (i - rowStart) = 32
re: base + i = 34 im: base + (i - rowStart) = 33
re: base + i = 35 im: base + (i - rowStart) = 34
{ rowBase: 1,
  colBase: 1,
  nrCols: 6,
  nrRows: 6,
  r:
   Float64Array [
     1.2629542848807933,
     -0.3262333607056494,
     1.3297992629225006,
     1.2724293214294047,
     0.4146414344564082,
     -1.5399500419037095,
     -0.9285670347135381,
     -0.2947204467905602,
     -0.005767172747536955,
     2.404653388857951,
     0.7635934611404596,
     -0.7990092489893682,
     -1.1476570092363514,
     -0.28946157368822334,
     -0.29921511789731614,
     -0.411510832795067,
     0.2522234481561323,
     -0.8919211272845686,
     0.43568329935571865,
     -1.237538421929958,
     -0.22426788527830935,
     0.37739564598170106,
     0.1333363608148414,
     0.8041895097449078,
     -0.057106774383808755,
     0.5036079722337261,
     1.085769362145687,
     -0.6909538396968303,
     -1.2845993538721883,
     0.04672617218835198,
     -0.23570655643950122,
     -0.5428882550102544,
     -0.4333103174567822,
     -0.6494716467962331,
     0.726750747385451,
     1.1519117540872 ],
  i:
   Float64Array [
     -0.42951310949188126,
     1.2383041008533804,
     -0.2793462818542693,
     1.7579030898107073,
     0.5607460908880562,
     -0.4527839725531578,
     -0.8320432961178319,
     -1.166570547084707,
     -1.0655905803882961,
     -1.563782051071005,
     1.1565369971501793,
     0.8320471285723897,
     -0.22732869142475534,
     0.2661373616721048,
     -0.3767027185836281,
     2.4413646288945894,
     -0.7953391172553718,
     -0.054877473711578625,
     0.2501413228541527,
     0.6182432935662469,
     -0.17262350264585732,
     -2.2239002740099374,
     -1.263614384970583,
     0.3587288959713519,
     -0.011045478465663564,
     -0.9406491626186084,
     -0.11582532215695436,
     -0.8149687088699175,
     0.24226348085968588,
     -1.4250983947324998,
     0.36594112304921983,
     0.2484126488725964,
     0.06528818167162072,
     0.01915639166027384,
     0.2573383771555333,
     0 ],
  coord: [Function: coord],
  colOfEx: [Function: colOfEx],
  setCol: [Function: setCol],
  upperBand: [Function: upperBand],
  lowerBand: [Function: lowerBand],
  packedUpper: [Function: packedUpper],
  packedLower: [Function: packedLower],
  toArr: [Function: toArr],
  slice: [Function: slice],
  setLower: [Function: setLower],
  setUpper: [Function: setUpper],
  real: [Function: real],
  imaginary: [Function: imaginary] }

I stand corrected, there actually several issues with that piece of code,
the

  • -1 is hardcoded default offset of fortran index base "1" that should be changed to rowBase
  • the rowStart needs to be corrected with the rowbase base + (i - (rowStart - rowBase)) (imaginary part)
  • the real part is not correct base + i should be same as above

Will correct today and do some tests, then release, thanks for finding the issue

version is now 1.0.12

Fixed