Skip to content

Comments

Type definition missing for matrixFrom* #3115#3146

Closed
Hudsxn wants to merge 6 commits intojosdejong:developfrom
Hudsxn:develop
Closed

Type definition missing for matrixFrom* #3115#3146
Hudsxn wants to merge 6 commits intojosdejong:developfrom
Hudsxn:develop

Conversation

@Hudsxn
Copy link
Contributor

@Hudsxn Hudsxn commented Feb 1, 2024

Added in the typescript definitions for matrixFromRows, matrixFromColumns and matrixFromFunction, Also created a type:

MatrixStorageFormat = 'dense' | 'sparse'

And swapped out other references, and created a type for the callback function on the matrixFromFunction:

export interface MatrixFromFunctionCallback<T extends (number | BigNumber)[]> {
  (value: T): number | BigNumber
}

Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Hudsxn , this looks good already!

Can you please add some tests to typescript-tests/testTypes.ts to verify that the types indeed work (and guard that they keep working in the future)?

* Create a dense matrix from vectors as individual rows. If you pass column vectors, they will be transposed (but not conjugated!)
* @param rows - a multi-dimensional number array or matrix
*/
matrixFromRows<T extends (number | BigNumber)[] | Matrix>(...rows: T[]): T[]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use MathCollection here? Looking at the examples of these functions, they also support nested arrays for example, not only a flat array.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, they only allow vectors, or vectors of one-tuples. I am trying to capture that in an update to polish off this PR to get it off the queue.

@josdejong
Copy link
Owner

@Hudsxn can you have a look at my feedback?

@josdejong
Copy link
Owner

Ping @Hudsxn

@gwhitney
Copy link
Collaborator

Superseded by #3371. Closing.

@gwhitney gwhitney closed this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants