feat: add Symmetric and Distance Matrix

[tpoisseau]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (72bdf3a) 71.27% compared to head (534c30b) 73.53%.

❗ Current head 534c30b differs from pull request most recent head f8c1518. Consider uploading reports for the commit f8c1518 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   71.27%   73.53%   +2.26%     
==========================================
  Files          34       36       +2     
  Lines        5295     5733     +438     
  Branches      850      937      +87     
==========================================
+ Hits         3774     4216     +442     
+ Misses       1516     1512       -4     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stropitek]

Thanks! Looked at the API and terminology used, not the implementation.

LGTM, I just have one issue. Is "side" a common / standard way to refer to the both row / column in a symmetric matrix? I'm wondering if we could find a better term.

My suggestion would be to:

• rename sideSize => diagonalSize (getter and param names). The term "diagonal" is more specific to square / symmetric matrices than "side".
• remove addSide and removeSide (we already have addRow, addColumn etc.)

[tpoisseau]

Thx for your return, I was struggled by "side" but found nothing better. diagonalSize seems better.

I do not agree on removing addSide / removeSide but open to better naming proposal. addRow / addColumn are in API theses Square Matrix specific for inheritance purpose, but name are wrong, because addRow add also a column and vice-versa. So we need proper methods and alias rows/columns for inheritance purpose.

What do you think for addCross / removeCross. (I think addDiagonal / removeDiagonal may not be comprehensive and could be understood wrongly)

[targos]

Can you move the new classes to separate files? matrix.js is already huge.

[stropitek]

I do not agree on removing addSide / removeSide but open to better naming proposal. addRow / addColumn are in API theses Square Matrix specific for inheritance purpose, but name are wrong, because addRow add also a column and vice-versa. So we need proper methods and alias rows/columns for inheritance purpose.

What do you think for addCross / removeCross. (I think addDiagonal / removeDiagonal may not be comprehensive and could be understood wrongly)

addDiagonal is confusing for sure. I'm ok with addCross, not 100% convinced but couldn't come up with something better.

[tpoisseau]

from @targos feedbacks: refactor to extends from AbstractMatrix done, (use matrix composition for implementation).

move to addCross / removeCross done.
complete some unit tests for coverage.

I will cut matrix.js file.

[tpoisseau]

done

[targos]

Generally LGTM.

I'm just confused with the upperRightValues/upperRightEntries API. What's the use case, especially for iterating outside of the matrix dimensions?

[tpoisseau]

Yeah, maybe theses iterators should be only on Symmetric/Distance Matrix

[targos]

What's the problem with iterating on a non-equilateral triangle?

[tpoisseau]

What's the problem with iterating on a non-equilateral triangle?

Ambiguity and so, your confusion, I have no real use case in mind about iterating like that upon a non-square matrix.

Maybe, upperRightValues should not assume a square algorithm instead, like that, no werid API like maxBorder and missValue.

[targos]

OK, maybe we only allow to call this method on a square matrix.

[tpoisseau]

TS2417: Class static side  typeof SymmetricMatrix  incorrectly extends base class static side  typeof AbstractMatrix 
The types returned by  zeros(...)  are incompatible between these types.
Type  SymmetricMatrix  is missing the following properties from type  Matrix :  removeColumn, removeRow, addColumn, addRow 

Sometime I hate Typescript, because AbstractMatrix factories return Matrix, I can't override them in my SymmetricMatrix because SymmetricMatrix is not a Matrix but an AbstractMatrix.

I think the proper way is theses static factories return type is AbstractMatrix, but it's a breaking change. and it will force casting when we would use specific Matrix method from factories

[targos]

Can't we somehow ask for the method to return "this" i.e. the current constructor.

[tpoisseau]

static zeros(rows: number, columns: number): this;
TS2526: A  this  type is available only in a non-static member of a class or interface.

Seems no

[targos]

I suggest we just remove the definitions from the AbstractMatrix class then.

[targos]

Or make it generic with a default to <Matrix>

[tpoisseau]

static zeros<M extends AbstractMatrix = Matrix>(
    rows: number,
    columns: number,
  ): M;

Allow me to not have error on inheritance but I must extends like that

  static zeros<M extends AbstractMatrix = SymmetricMatrix>(
    diagonalSize: number,
  ): M;

I can't restrict the extends, so type can be set to Matrix but it will not return a Matrix 😬.

TS2417: Class static side  typeof SymmetricMatrix  incorrectly extends base class static side  typeof AbstractMatrix 
The types returned by  zeros(...)  are incompatible between these types.
Type  SymmetricMatrix  is not assignable to type  M 
 M  could be instantiated with an arbitrary type which could be unrelated to  SymmetricMatrix 
 ```
 Error message if I force M extends to SymmetricMatrix

I think it's the best we can do without breaking change.

[tpoisseau]

What bother me the most is the error is on class, not on method. so I cannot ts-except-error or it will apply on all errors on class. (we can't ignore specific error)

[targos]

Let's make it return unknown or any then (in the abstract class)?
The class that extends it will specify the type.

[tpoisseau]

I'll keep like that, I specified in doc to not specify the generic.

But we can create an issue for review factory functions and move them into Matrix instead AbstractMatrix.