R-js / blasjs

Pure Javascript manually written :ok_hand: implementation of BLAS, Many numerical software applications use BLAS computations, including Armadillo, LAPACK, LINPACK, GNU Octave, Mathematica, MATLAB, NumPy, R, and Julia.
MIT License
290 stars 21 forks source link

Question about slice function #3

Closed Schultzer closed 5 years ago

Schultzer commented 5 years ago

Hi,

First of all thanks for all this work. I have a question about https://github.com/R-js/blasjs/blob/b994277970ce64c90330859fdb65acc9194a14b6/src/lib/f_func.ts#L425

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

In the first round base + (i - rowStart) == -1 which result in a undefinedbehavior 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?

jacobbogers commented 5 years ago

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

Schultzer commented 5 years ago

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] }
jacobbogers commented 5 years ago

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

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

jacobbogers commented 5 years ago

version is now 1.0.12

Fixed