Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed MathArray TS type #3099

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Amolition
Copy link

Fixed type of MathArray to prevent typescript erroring for any n-dimensional array (previously only worked for n<=2). Solution uses a conditionally recursive type.

Fixed type of MathArray to prevent typescript erroring for any n-dimensional array (previously only worked for n<=2)
@Amolition
Copy link
Author

Addresses issue #3098

@josdejong
Copy link
Owner

Thanks @Amolition for this fix, this makes sense. Can you add a test in the file /test/typescript-tests/testTypes.ts to ensure that n-dimensional arrays are supported?

@josdejong
Copy link
Owner

I see that the build-and-test script fails to run, can you also look into that? Run npm install && npm run build-and-test on your machine.

@Amolition
Copy link
Author

Hi @josdejong, thanks for your feedback. I think the build-and-test errors were down to some minor failures of some of the typescript tests due to incorrect type inferences.

However, looking deeper into the code, I'm not actually sure any longer which functions would support n-dimensional arrays. I initially assumed that all did, but on further inspection it seems that the multiply function only supports up to matrix multiplication, i.e. does not generalise to rank n > 2 tensors. It may also be the case that other functions are assuming that arrays are always 2 or fewer dimensions, although I haven't checked this thoroughly.

In light of this, supporting n-dimensional arrays is not just a matter of changing the Typescript definition, and would require checking every function to ensure they work with tensors and re-write them if not (or flag them as incompatible with n > 2 dimensions). This is obviously a much bigger task, so I'm not sure how best to proceed?

I'm new to open source development and not an especially talented mathematician so please take my analysis with a grain of salt, and let me know if you think I got anything wrong in the above assessment!

Thanks

Amol

@josdejong
Copy link
Owner

That is a very good point Amolition. In general, the Matrix type and the nested Arrays can be n-dimensional in mathjs, but not all functions do support n-d matrices. In the long term, we want to auto-generate types from the source code, that should give accurate type definitions. For the time being, we still maintain the types manually.

I think there are three possible scenarios here:

  1. Right now, the types are too conservative. When you use mathjs in TypeScript, you simply don't know that you can use n-d matrices.
  2. The cheapest way to improve on this is to define Matrix and Array as being n-dimensional. Only doing that will give no compile-time knowledge about whether an arbitrary function can support this or not, and only throwing a runtime error when executing that. This is similar to say round(x: number, decimals: number) not reflecting in the type that decimals must be a positive integer. Or the type of pow(value: Matrix, exp: number) not reflecting that the matrix must be square. Etc.
  3. We can go through every available function and define whether it supports n-d matrices or not. That should improve type definitions, though I think it may also introduce difficulties in that people need to do type casting to explain TS that some arbitrary array really is only 2-d and not n-d. That will be considerably more work.

At this point I think that approach (2) would be an improvement over the current situation (1). And I am not sure whether we should try out option at this moment (3), or better await auto-generating types.

What do you think?

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.

None yet

2 participants